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.

Issue fork drupal-3332442

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

Gauravvv opened merge request !3395

gauravvvv’s picture

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

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

Reviewed MR 3395.
Confirmed the nesting looks correct.
colors replaced with variables.
Fact that the .css is unchanged shows nothing should have broken.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Status: Reviewed & tested by the community » Needs work

Left two comments in MR. Also, issue summary mentions "Please post before/after screenshots and make sure they look the same." so that should happen too. Since focus/hover is in the CSS that should be accounted for in the screenshots.

Enjoy this rare instance of me requesting screenshots instead of yelling about there being too many of them.

gauravvvv’s picture

gauravvvv commented

Also, issue summary mentions "Please post before/after screenshots and make sure they look the same." so that should happen too. Since focus/hover is in the CSS that should be accounted for in the screenshots.

@bnjmnm As CSS file remains unchanged, I don't think so we need screenshots here.

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review

Addressed all the feedbacks. Please review

smustgrave’s picture

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

Changes look good. Think the failure was random but ran again to be sure.

Will remove credit from myself as I did a rebase to make sure they passed. Will let committers decide to add back or not.

  • lauriii committed 93f6a239 on 10.1.x 
    Issue #3332442 by Gauravvv, smustgrave, bnjmnm: Refactor Claro's skip-...
lauriii’s picture

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

Committed 93f6a23 and pushed to 10.1.x. Thanks!

lauriii closed merge request !3395

Status: Fixed » Closed (fixed)

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