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
#13 Screenshot 2023-03-15 at 7.58.04 AM.png213.13 KBGauravvvv

Issue fork drupal-3332363

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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

morganlyndel created an issue. See original summary.

morganlyndel’s picture

morganlyndel CreditAttribution: morganlyndel commented
Issue summary: View changes

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

bspeare opened merge request !3381

bspeare’s picture

bspeare CreditAttribution: bspeare commented
Version: 10.0.x-dev » 10.1.x-dev

bspeare closed merge request !3381

bspeare opened merge request !3383

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

Gauravvvv’s picture

Gauravvvv CreditAttribution: Gauravvvv at Axelerant for Drupal India Association commented
Status: Active » Needs review

I have improved nesting and added margin-inline-start & margin-inline-end, so we don't need 

[dir="rtl"] .item-list ul {
  margin: 0.25em 1.5em 0.25em 0;
}

anymore.

Please review

smustgrave’s picture

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

Seems more nesting could be done I think.

Also does [dir="rtl"] need to be replaced?

Gauravvvv’s picture

Gauravvvv CreditAttribution: Gauravvvv at Axelerant for Drupal India Association commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Needs work

Issue summary mentions needing before/after screenshots to show nothing broke.

Code wise everything looks fine.

Gauravvvv’s picture

Gauravvvv CreditAttribution: Gauravvvv at Axelerant for Drupal India Association commented
Status: Needs work » Needs review
FileSize
Screenshot 2023-03-15 at 7.58.04 AM.png213.13 KB


After patch:

Added after patch screenshot, please review

smustgrave’s picture

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

Will let committer decide but seems difficult to read. But not sure what could be done.

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

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel as a volunteer commented
Status: Reviewed & tested by the community » Needs review

Did a bit of work on this.

The first thing I noticed was image referenced in list-style-image: url(../../images/menu-collapsed.png); did not exist.

This image should get called when there's a menu. Note that the menu just shows up as an unordered list, but the images (which were small triangles) didn't show. So, I removed them from being called.

I also added spacing between the code blocks and converted the REM unit to PX (as it'll be auto converted by PostCSS.

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

smustgrave’s picture

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

Tested on modules page and didn't see any issues.

  • nod_ committed bf266299 on 10.1.x 
    Issue #3332363 by Gauravvvv, mherchel, smustgrave: Refactor Claro's...
nod_’s picture

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

Committed bf26629 and pushed to 10.1.x. Thanks!

Thank you for your assistance on this issue VladimirAus

Starting March 2023, simple rerolls, rebases, or merges will no longer receive issue credit. Only rerolls that address a merge conflict will be credited, and the merge conflict that was resolved must be documented in the text of an issue comment.

To receive credit for contributing to this issue, assist with other outstanding tasks or unaddressed feedback.

See the issue credit guidelines for more information.

Status: Fixed » Closed (fixed)

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