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
#8 Screenshot 2023-02-14 at 11.39.40 AM.png56.21 KBgauravvvv
#7 Screenshot 2023-02-14 at 11.25.49 AM.png92.81 KBgauravvvv
#7 Screenshot 2023-02-14 at 11.25.11 AM.png69.58 KBgauravvvv

Issue fork drupal-3332459

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.

bspeare’s picture

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

royalpinto007
he/him
Primary language English
commented
Assigned: Unassigned » royalpinto007
Status: Active » Needs work

royalpinto007 opened merge request !3461

royalpinto007’s picture

royalpinto007
he/him
Primary language English
commented
Status: Needs work » Needs review

I've built the file and ran some local tests, and it looks like everything is working as expected.
No errors were encountered during testing.

cilefen’s picture

cilefen commented
Status: Needs review » Needs work

The issue summary says:

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

gauravvvv’s picture

gauravvvv commented
StatusFileSize
new Screenshot 2023-02-14 at 11.25.11 AM.png69.58 KB
new Screenshot 2023-02-14 at 11.25.49 AM.png92.81 KB

Before patch:

After patch #4:

Some selectors are changed, which needs to fixed. moving to NW.

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review
StatusFileSize
new Screenshot 2023-02-14 at 11.39.40 AM.png56.21 KB

I have corrected the selectors and improved the nesting. Updated MR for same. Please review

After patch:

royalpinto007’s picture

royalpinto007
he/him
Primary language English
commented
Assigned: royalpinto007 » Unassigned
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Nesting looks good
Variables are being used
before/after screenshots added.

nod_’s picture

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

The specificity of the selector is changed (increased) by the patch and it's not clear that this is necessary, if there are no other changes or justification that'd be a won't fix for me.

gauravvvv’s picture

gauravvvv commented

The specificity of the selector is changed (increased) by the patch

Specificity is updated just to improve nesting.

nod_’s picture

nod_
Primary language French
Location Lille
commented

right, but that can cause issues with projects that override just a few things.

My understanding is that we'd use nesting if that simplify generating the same output as before. In this case the code before refactoring didn't use selectors that could be simplified with nesting, so nesting shouldn't be necessary.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs review » Needs work

+1 for #11. We should not increase the selector specificity here.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs work » Closed (works as designed)

Thanks, since this is the only change in this MR, I'm closing as works a designed.

See you in the other CSS issues :)

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Closed (works as designed) » Needs work

Actually we could use some logical properties in this file. Not that we have a need for RTL styles but it can't hurt :)

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review

Updated MR with logical properties and removed the increased selector specificity. please review

guru2023’s picture

guru2023 commented
Assigned: Unassigned » guru2023
guru2023’s picture

guru2023 commented
Assigned: guru2023 » Unassigned
smustgrave’s picture

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

Changes look good @Gauravvvv

  • nod_ committed 28e4c1f5 on 10.1.x 
    Issue #3332459 by Gauravvvv, royalpinto007: Refactor Claro's system-...
nod_’s picture

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

Committed 28e4c1f and pushed to 10.1.x. Thanks!

Status: Fixed » Closed (fixed)

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