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
#13 Screen Shot 2023-03-13 at 12.28.27 PM.png228.12 KBsmustgrave
#13 Screen Shot 2023-03-13 at 12.28.17 PM.png183.15 KBsmustgrave
#12 interdiff-9_12.txt4.34 KBGauravvvv
#12 3332419-12.patch9.36 KBGauravvvv
#9 Screenshot 2023-03-01 at 11.54.55 AM.png379.63 KBGauravvvv
#9 interdiff-7_9.txt5.16 KBGauravvvv
#9 3332419-9.patch7.97 KBGauravvvv
#7 3332419-7.patch6.85 KBGauravvvv

Issue fork drupal-3332419

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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Stockfoot created an issue. See original summary.

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

Gauravvv opened merge request !3236

Gauravvvv’s picture

Gauravvvv CreditAttribution: Gauravvvv at Axelerant for Drupal India Association commented
Status: Active » Needs review
smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

Appears to have issues in the MR.

Gauravvvv’s picture

Gauravvvv CreditAttribution: Gauravvvv at Axelerant for Drupal India Association commented
Version: 10.0.x-dev » 10.1.x-dev
Gauravvvv’s picture

Gauravvvv CreditAttribution: Gauravvvv at Axelerant for Drupal India Association commented
Status: Needs work » Needs review
FileSize
3332419-7.patch6.85 KB
smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Needs work 
.messages a {
  text-decoration: underline;
  color: var(--messages__link-color);
}

.messages a:hover {
  color: var(--messages__link--hover-color);
}

.messages pre {
  margin: 0;
}

think more nesting could be done here.

Gauravvvv’s picture

Gauravvvv CreditAttribution: Gauravvvv at Axelerant for Drupal India Association commented
Status: Needs work » Needs review
FileSize
3332419-9.patch7.97 KB
interdiff-7_9.txt5.16 KB
Screenshot 2023-03-01 at 11.54.55 AM.png379.63 KB

Addressed #8, make few more changes. Attached patch, interdiff and after patch screenshot. please review

After patch

smustgrave’s picture

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

Nesting seems good now

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/themes/claro/css/components/messages.pcss.css
@@ -65,63 +75,37 @@
+  @nest .messages--error & {
...
+  @nest .messages--status & {
...
+  @nest .messages--warning & {
...
+  @nest [dir="rtl"] & {

This would probably be easier to read if we combined these with the similar selectors few lines above to something like this:

.messages--error {
  .messages__header {
  }
}
Gauravvvv’s picture

Gauravvvv CreditAttribution: Gauravvvv at Axelerant for Drupal India Association commented
Status: Needs work » Needs review
FileSize
3332419-12.patch9.36 KB
interdiff-9_12.txt4.34 KB

Addressed #11, attached patch and interdiff for same. please review

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Reviewed & tested by the community
FileSize
Screen Shot 2023-03-13 at 12.28.17 PM.png183.15 KB
Screen Shot 2023-03-13 at 12.28.27 PM.png228.12 KB

Here's a few screenshots but appears everything is still functional

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: 3332419-12.patch, failed testing. View results

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ at Eclipse Foundation commented
Status: Needs work » Reviewed & tested by the community
nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ at Eclipse Foundation commented
Status: Reviewed & tested by the community » Fixed

Works fine for RTL, keeping @nest is ok. As explained by mherchel, there will be an automated way to move to the new syntax.

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

  • nod_ committed 7cb4919f on 10.1.x 
    Issue #3332419 by Gauravvvv, smustgrave, lauriii: Refactor Claro's...
Gauravvvv’s picture

Gauravvvv CreditAttribution: Gauravvvv at Axelerant for Drupal India Association commented
Status: Reviewed & tested by the community » Fixed

Updating status. Still showing RTBC.

Status: Fixed » Closed (fixed)

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