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
#27 core-3332428-23.patch886 bytesnod_
#20 3332428-20.patch2.3 KBkostyashupenko
#19 3332428-nr-bot.txt85 bytesneeds-review-queue-bot
#18 3332428-18.patch2.41 KBstanzin
#13 interdiff.txt1.6 KBlauriii
#13 3332428-14.patch2.13 KBlauriii
#10 interdiff.txt1.97 KBlauriii
#10 3332428-10.patch2.13 KBlauriii
#8 interdiff-5_8.txt616 bytesgauravvvv
#8 3332428-8.patch843 bytesgauravvvv
#5 3332428-5.patch502 bytesgauravvvv

Issue fork drupal-3332428

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.

Gauravvv opened merge request !3389

Gauravvv closed merge request !3389

gauravvvv’s picture

gauravvvv CreditAttribution: gauravvvv at Axelerant for Drupal India Association commented
Status: Active » Needs review
StatusFileSize
new 3332428-5.patch502 bytes

Improved nesting of selectors. Please verify

smustgrave’s picture

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

Not sure if .region-header > .page-title {

But will let committer decide.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Reviewed & tested by the community » Needs work

Using the @nest syntax would probably maker sense with the .region-header > .page-title selector.

gauravvvv’s picture

gauravvvv CreditAttribution: gauravvvv at Axelerant for Drupal India Association commented
Status: Needs work » Needs review
StatusFileSize
new 3332428-8.patch843 bytes
new interdiff-5_8.txt616 bytes

Addressed #7, attached patch and interdiff. please review

smustgrave’s picture

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

Thanks looks good now!

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Version: 10.0.x-dev » 10.1.x-dev
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 3332428-10.patch2.13 KB
new interdiff.txt1.97 KB

Thoughts on this? 😇

smustgrave’s picture

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

Looks good code wise. Not aware of the @nest will have to look at that up.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/themes/claro/css/components/page-title.css
@@ -18,20 +18,20 @@
+   */

I'm not a fan of the complied file resulting in the comment appearing in a different block than what it is commenting on. I know it's not in the file being edited but it could still be inspected in browser tools. If there isn't a way to tweak the syntax to get that working in this issue scope, perhaps there should be another issue to address comment placement in the built file and this can be postponed on that.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new 3332428-14.patch2.13 KB
new interdiff.txt1.6 KB

Is there an inspector that shows CSS comments? 🤔 This should address the feedback but I'm not sure it's a good use of our time to optimize how comments appear in the generated CSS file.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: 3332428-14.patch, failed testing. View results

gauravvvv’s picture

gauravvvv CreditAttribution: gauravvvv at Axelerant for Drupal India Association commented
Status: Needs work » Reviewed & tested by the community

Unrelated failure. restoring status

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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 13: 3332428-14.patch, failed testing. View results

stanzin’s picture

stanzin CreditAttribution: stanzin as a volunteer commented
Status: Needs work » Needs review
StatusFileSize
new 3332428-18.patch2.41 KB

How's this for D11?

needs-review-queue-bot’s picture

needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commented
Status: Needs review » Needs work
StatusFileSize
new 3332428-nr-bot.txt85 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

kostyashupenko’s picture

kostyashupenko
Primary language English
Location Omsk
CreditAttribution: kostyashupenko at Skilld commented
Status: Needs work » Needs review
StatusFileSize
new 3332428-20.patch2.3 KB
smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Reviewed & tested by the community
quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented

I'm triaging RTBC issues. I read the IS and the comments. I didn't find any unanswered questions or other work to do.

Leaving at RTBC.

  • nod_ committed 59d11a92 on 11.x 
    Issue #3332428 by Gauravvvv, lauriii, kostyashupenko, smustgrave, bnjmnm...

  • nod_ committed 6ed8194e on 10.2.x 
    Issue #3332428 by Gauravvvv, lauriii, kostyashupenko, smustgrave, bnjmnm...

  • nod_ committed 71ef375f on 11.x 
    Issue #3332428 by Gauravvvv, lauriii, kostyashupenko, smustgrave, bnjmnm...

  • nod_ committed 035581b2 on 10.2.x 
    Issue #3332428 by Gauravvvv, lauriii, kostyashupenko, smustgrave, bnjmnm...
nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ as a volunteer commented
Status: Reviewed & tested by the community » Fixed
StatusFileSize
new core-3332428-23.patch886 bytes

Committed 59d11a9 and pushed to 11.x. Thanks!

The patch had an issue it was setting the top margin instead of the bottom margin. Fixed on commit, applied patch here.

Status: Fixed » Closed (fixed)

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