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

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 commented
Status: Active » Needs review
smustgrave’s picture

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

Appears to have issues in the MR.

gauravvvv’s picture

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

gauravvvv commented
Status: Needs work » Needs review
StatusFileSize
new 3332419-7.patch6.85 KB
smustgrave’s picture

smustgrave 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 commented
Status: Needs work » Needs review
StatusFileSize
new 3332419-9.patch7.97 KB
new interdiff-7_9.txt5.16 KB
new 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 commented
Status: Needs review » Reviewed & tested by the community

Nesting seems good now

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/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 commented
Status: Needs work » Needs review
StatusFileSize
new 3332419-12.patch9.36 KB
new interdiff-9_12.txt4.34 KB

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

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new Screen Shot 2023-03-13 at 12.28.17 PM.png183.15 KB
new 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
commented
Status: Needs work » Reviewed & tested by the community
nod_’s picture

nod_
Primary language French
Location Lille
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 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.