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-3332707

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:

Comments

Stockfoot created an issue. See original summary.

gauravvvv’s picture

Version: 10.0.x-dev » 10.1.x-dev

gauravvvv’s picture

Status: Active » Needs review
smustgrave’s picture

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

Seems to have two instances of

width: 240px;
width: 15rem;

Which one should it be?

Also 8 instances of [dir="rtl"] should those be replaced?

Since the .css file changed this will need before/after screenshots too

gauravvvv’s picture

Status: Needs work » Needs review
StatusFileSize
new253.53 KB
new308.62 KB

I have removed the duplicate width property. Also rtl styling is also refactored. please review

Before patch

After patch

smustgrave’s picture

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

Thanks!

nod_’s picture

Status: Reviewed & tested by the community » Needs work

Appart from the selector change, patch looks good.

The selector change makes the tour button show up on pages where there are no tour for example.

gauravvvv’s picture

Status: Needs work » Needs review

Updated the selector.

santosh_verma’s picture

Tested the MR #3425

1. Nesting added
2. Normal css replaced with Logical Properties
3. Addressed the comment #9, Selector change reverted in the commit 3

SS1

Tested UI manually Nothing looks like broken
UI

RTBC +1

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Open thread has been addressed.

  • nod_ committed 3d86f4c4 on 10.1.x
    Issue #3332707 by Gauravvvv, Santosh_Verma, smustgrave: Refactor Claro's...
nod_’s picture

Status: Reviewed & tested by the community » Fixed

Committed 3d86f4c and pushed to 10.1.x. Thanks!

Status: Fixed » Closed (fixed)

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