Problem/Motivation

This is a child of #3324398: [META] Update Claro CSS with new coding standards and part of #3254529: [PLAN] Drupal CSS Modernization Initiative.

Steps to reproduce

The stylesheet at https://git.drupalcode.org/project/drupal/-/blob/10.0.x/core/themes/clar... needs to be refactored to make use of modern CSS and Drupal core's PostCSS tooling.

@todo: Add clear testing instructions to test this manually on the UI.

Proposed resolution

  • Use CSS Logical Properties where appropriate.
  • Use CSS nesting where appropriate.
  • Use existing variables (variables.pcss.css) where appropriate. Follow the proposed Drupal CSS coding standards to name the variables.
    • Add a comment when there's a value where there is not a variable like font-size: 1.23rem; /* @todo One off value. */
    • When possible, set variables at the root of the component and then map them to global theme variables:
      .entity-meta {
          --entity-meta-title-font-size: var(--font-size-h5);

          ... more style
        }

        .entity-meta__title {
          font-size: var(--entity-meta-title-font-size);
        }

Out of scope

  • Changing CSS classes
  • Drupal 9 patches

User interface changes

None. There should be no visual differences.
Please post before/after screenshots and make sure they look the same.

CommentFileSizeAuthor
#26 3332425-26.patch3.25 KBgauravvvv
#24 3332425-24.patch4.72 KB_pratik_
#22 interdiff_16-22.txt771 bytesakram khan
#22 3332425-22.patch4.7 KBakram khan
#16 3332425-16.patch4.71 KBakshay kashyap
#8 Screenshot 2023-03-01 at 12.46.30 PM.png346.81 KBgauravvvv
#5 3332425-5.patch3.25 KBgauravvvv

Issue fork drupal-3332425

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

Stockfoot created an issue. See original summary.

stockfoot’s picture

stockfoot commented
Issue tags: +2022-GSOC-CSS, +CSS

Gauravvv made their first commit to this issue’s fork.

gauravvvv’s picture

gauravvvv commented
Version: 10.0.x-dev » 10.1.x-dev
gauravvvv’s picture

gauravvvv commented
Status: Active » Needs review
StatusFileSize
new 3332425-5.patch3.25 KB
gauravvvv’s picture

gauravvvv commented

Only changed color variables, So I don't think we need screenshots.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative, +Needs screenshots

Changes look like.

But since the .css file changed can we get before/after screenshots please.

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review
StatusFileSize
new Screenshot 2023-03-01 at 12.46.30 PM.png346.81 KB

After patch

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs screenshots

Thanks

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: 3332425-5.patch, failed testing. View results

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Reviewed & tested by the community

Re-storing status, unrelated failure.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: 3332425-5.patch, failed testing. View results

smustgrave’s picture

smustgrave commented
Status: Needs work » Reviewed & tested by the community

Random failure

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 5: 3332425-5.patch, failed testing. View results

akshay kashyap’s picture

akshay kashyap commented

@Gauravvvv we could have created variables for the borders as well.

akshay kashyap’s picture

akshay kashyap commented
StatusFileSize
new 3332425-16.patch4.71 KB

Created a new patch. Added border-variables. Please review it.

akshay kashyap’s picture

akshay kashyap commented
Status: Needs work » Needs review
akshay kashyap’s picture

akshay kashyap commented

Guys, can you please explain why I am getting this CCF my patch is passed, and after clicking on CCF I am not able to see the exact error which I can resolve, please help me to understand this so that I can verify the same and it will be helpful for me in the future, Really appreciated if you would help me in this.

Thanks

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Random failure in #5

@Akshay kashyap not sure we need a variable for 0. Could be wrong but don't see anywhere else in the theme we use variables for 0.

The last submitted patch, 5: 3332425-5.patch, failed testing. View results

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Possible a dependency update changed the compiled css => CCF, let's reroll this one. feeling lucky, adding the tag :)

akram khan’s picture

akram khan
Primary language English
Location Cuttack, Odisha
commented
StatusFileSize
new 3332425-22.patch4.7 KB
new interdiff_16-22.txt771 bytes

added updated patch to fix CCF #16

nod_’s picture

nod_
Primary language French
Location Lille
commented 
+++ b/core/themes/claro/css/components/modules-page.css
@@ -57,3 +57,3 @@
+  border: var(-modules-page-system-modules-extrasmall-border-size);

missing a - here.

_pratik_’s picture

_pratik_
Primary language English
Location Indore
commented
Status: Needs work » Needs review
StatusFileSize
new 3332425-24.patch4.72 KB

Fix for mention in #23
thanks

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Needs work
Issue tags: -Needs reroll

It looks like we can improve the nesting


.system-modules, .locale-translation-status-form {
  & tr.even, & tr.odd {
    border: black;
  }
}

and so on.

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review
StatusFileSize
new 3332425-26.patch3.25 KB

Reposted patch #5. removed unnecessary patches. please review

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Reviewed & tested by the community

Patch was RTBC in #9, since we have a RTBC and screenshot for that one I'd rather commit this and address the border in a followup if needed.

Using vars for border is a good idea too. It's just that I'd rather commit this one than wait a few days/weeks for the additional scope to be done.

  • nod_ committed cc5e3477 on 10.1.x 
    Issue #3332425 by Gauravvvv: Refactor Claro's modules-page stylesheet
nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Fixed

Committed cc5e347 and pushed to 10.1.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.