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
#11 3332444-rtl-after.png454.55 KBpradipmodh13
#11 3332444-rtl-before.png450.24 KBpradipmodh13
#11 3332444-after-patch.png481.94 KBpradipmodh13
#11 3332444-before-patch.png476.48 KBpradipmodh13

Issue fork drupal-3332444

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.

bspeare’s picture

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

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

Gauravvv opened merge request !3460

gauravvvv’s picture

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

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs review » Needs work

This is looking really great! I left some comments in the MR, plus we need to make sure that each code block is separated by a blank line.

gauravvvv’s picture

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

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

Since the .css file has changed can we get screenshots please.

pradipmodh13’s picture

pradipmodh13 commented
Assigned: Unassigned » pradipmodh13
pradipmodh13’s picture

pradipmodh13 commented
Assigned: pradipmodh13 » Unassigned
pradipmodh13’s picture

pradipmodh13 commented
StatusFileSize
new 3332444-before-patch.png476.48 KB
new 3332444-after-patch.png481.94 KB
new 3332444-rtl-before.png450.24 KB
new 3332444-rtl-after.png454.55 KB

Hello @smustgrave,
As requested I am attaching here after and before screenshot.
Admin design looks fine after applying patch.

gauravvvv’s picture

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

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

Changes look good.

bnjmnm’s picture

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

There are two unaddressed items from an earlier review, and I additional requested to remove the @nest syntax as it will not be part of the CSS spec. Nesting will still work, but it doesn't need the at-rule anymore.

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review

If we don't use @nest then the output is broken

  [dir="rtl"] & {
    transform: scaleX(-1);
  }

Output:

.admin-item__link::before {
  position: absolute;
  inset-block-start: 0;
  inset-inline-start: 0;
  display: block;
  width: 1em;
  height: 1.5em;
  content: "";
  background: transparent no-repeat 50% 50%;
  background-image: url("data:image/svg+xml,%3csvg width='9' height='14' xmlns='http://www.w3.org/2000/svg'%3e%3cpath d='M1.71.314L.29 1.723l5.302 5.353L.289 12.43l1.422 1.408 6.697-6.762z' fill='%23003ecc'/%3e%3c/svg%3e");

  .admin-item__link::before {
    transform: scaleX(-1);
  }
}

Addressed other points.

Gauravvvv opened merge request !3978

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

MR seems unmergable.

gauravvvv’s picture

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

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -2022-GSOC-CSS, -CSS, -Needs screenshots

Reran the MR tests and all green.

Cleaning up the tags some.

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

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

  • lauriii committed f72375ae on 11.x 
    Issue #3332444 by Gauravvvv, pradipmodh13, smustgrave, mherchel, bnjmnm...
lauriii’s picture

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

Committed f72375a and pushed to 11.x. Thanks!

lauriii closed merge request !3978

Status: Fixed » Closed (fixed)

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