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.

Before Patch:

After patch:

CommentFileSizeAuthor
#11 Screenshot 2023-02-17 at 10.40.39 AM.png54.98 KBgauravvvv
#11 Screenshot 2023-02-17 at 10.38.52 AM.png63.24 KBgauravvvv
#6 interdiff_4-6.txt426 bytesakram khan
#6 3332698-6.patch5.4 KBakram khan

Issue fork drupal-3332698

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.

gauravvvv’s picture

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

gauravvvv commented
Status: Active » Needs review

1. I have removed 

[dir="rtl"] td.checkbox,
[dir="rtl"] th.checkbox {
  /* This is required to win over specificity of [dir="rtl"] td */
  text-align: center;
}

As above code doing the same 

td.checkbox,
th.checkbox {
  text-align: center;
}

2. Improved nesting. Please review

Gauravvv opened merge request !3402

smustgrave’s picture

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

CI failures.

akram khan’s picture

akram khan
Primary language English
Location Cuttack, Odisha
commented
StatusFileSize
new 3332698-6.patch5.4 KB
new interdiff_4-6.txt426 bytes

fixed CI failures

akram khan’s picture

akram khan
Primary language English
Location Cuttack, Odisha
commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

For consistency fix was started in MR on a valid branch and should be continued there.

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs screenshots

Issue summary mentions needing screenshots can you add those please.

gauravvvv’s picture

gauravvvv commented
Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new Screenshot 2023-02-17 at 10.38.52 AM.png63.24 KB
new Screenshot 2023-02-17 at 10.40.39 AM.png54.98 KB

Before Patch:

After patch:

smustgrave’s picture

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

Thanks @Gauravvv!

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

  • lauriii committed 5b49fe36 on 10.1.x 
    Issue #3332698 by Gauravvv, Akram Khan, smustgrave: Refactor Claro's...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Fixed

Made few additional cleanups in the MR to combine rules more efficiently, and removed one instance where nesting was used where it seemed to make the rule appear unnecessarily complex because of the nesting.

Committed 5b49fe3 and pushed to 10.1.x. Thanks!

lauriii closed merge request !3402

Status: Fixed » Closed (fixed)

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