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

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.

bspeare’s picture

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

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

gauravvvv’s picture

Status: Active » Needs review
StatusFileSize
new392.06 KB
new390.59 KB

There should be no visual differences. here is the before and after patch screenshot. Please review

Before patch:

After patch:

smustgrave’s picture

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

haha like you knew I was going to ask for screenshots!

Everything looks good.
Nesting seems correct and I don't see any hardcoded values

lauriii’s picture

Status: Reviewed & tested by the community » Needs work

Posted some feedback on the MR. Thank you for working on these issues! 😊

gauravvvv’s picture

Status: Needs work » Needs review
smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Appears the changes requested have been implemented.

lauriii’s picture

Status: Reviewed & tested by the community » Needs review

There are some styles RTL styles that are depending on #3332461: Refactor Claro's system-admin--status-report stylesheet. I'm wondering if we should merge these two issues so that we can make sure everything works as expected?

smustgrave’s picture

Status: Needs review » Needs work

Closed #3332461: Refactor Claro's system-admin--status-report stylesheet so work there should be combined here. Didn't need to move credit as they are already here.

gauravvvv’s picture

Status: Needs work » Needs review
StatusFileSize
new315.91 KB

Merged system-admin--status-report.css and system-status-report.css Please review

After patch

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Thanks for the quick turnaround!

nod_’s picture

Status: Reviewed & tested by the community » Needs work

need rebasing

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

rassoni’s picture

Status: Needs work » Needs review

Rebase the branch. Please review.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Rebase look good.

  • nod_ committed 2f8c7039 on 10.1.x
    Issue #3332469 by Gauravvvv, Rassoni, smustgrave, lauriii: Refactor...
nod_’s picture

Status: Reviewed & tested by the community » Fixed

Patch fixes some layout issues in RTL, nice.

Committed 2f8c703 and pushed to 10.1.x. Thanks!

Status: Fixed » Closed (fixed)

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