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
#8 interdiff-3_7.txt589 bytesgauravvvv
#7 3332445-7.patch571 bytesgauravvvv
#3 3332445-2.patch595 bytesgauravvvv

Issue fork drupal-3332445

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.

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

gauravvvv’s picture

gauravvvv commented
Status: Active » Needs review
StatusFileSize
new 3332445-2.patch595 bytes

Refactored system-admin--links.pcss.css file.

1. Improved nesting of selectors.
2. No changes in system-admin--links.css file, so we're good to review.

bspeare’s picture

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

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

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Change looks good.

All nesting is done

.css is unchanged so no screenshots are needed.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/themes/claro/css/components/system-admin--links.pcss.css
@@ -13,11 +13,15 @@
+small {
+  & .admin-link {

I think this would be more readable if we had this only nested on two levels. First level would be small .admin-link and then we have the &:before and &:after inside it.

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review
StatusFileSize
new 3332445-7.patch571 bytes

Thankyou for quick feedback @Lauri. I agree with you. I have updated the patch with required changes. please review

gauravvvv’s picture

gauravvvv commented
StatusFileSize
new interdiff-3_7.txt589 bytes

Attached interdiff for same.

smustgrave’s picture

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

  • lauriii committed 7c9320bf on 10.1.x 
    Issue #3332445 by Gauravvv, smustgrave: Refactor Claro's system-admin--...
lauriii’s picture

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

Committed 7c9320b and pushed to 10.1.x. Thanks!

Status: Fixed » Closed (fixed)

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