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
#43 After-refactor-tablesort-indicator.png13.49 KBmeeni_dhobale
#43 Before-refactor-tablesort-indicator.png20.29 KBmeeni_dhobale
#43 RTL-After-refactor-tablesort-indicator.png26.25 KBmeeni_dhobale
#43 RTL-Before-refactor-tablesort-indicator.png16.93 KBmeeni_dhobale
#41 after-rtl-wo-is-forced-colors-tables-sortable-heading-Screenshot_20231219_195443.png7.63 KBshmy
#41 before-rtl-wo-is-forced-colors-tables-sortable-heading-Screenshot_20231219_195647.png7.63 KBshmy
#41 after-rtl-wo-is-tables-sortable-heading-Screenshot_20231219_174544-cropped.png290.81 KBshmy
#41 before-rtl-wo-is-tables-sortable-heading-Screenshot_20231219_175340-cropped.png290.81 KBshmy
#41 after-forced-colors-tables-sortable-heading-Screenshot_20231219_181302-cropped.png290.81 KBshmy
#41 before-forced-colors-tables-sortable-heading-Screenshot_20231219_181526-cropped.png290.81 KBshmy
#41 after-tables-sortable-heading-Screenshot_20231219_180952-cropped.png290.81 KBshmy
#41 before-tables-sortable-heading-Screenshot_20231219_181436-cropped.png290.81 KBshmy
#29 3332701-29.patch9.75 KBgauravvvv
#26 Screenshot 2023-04-15 at 7.57.21 PM.png24.16 KBsmustgrave
#26 Screenshot 2023-04-15 at 7.57.18 PM.png27.83 KBsmustgrave
#24 Content___Drupal.png82.76 KBmherchel
#15 Screenshot 2023-02-21 at 10.31.34 AM.png97.71 KBgauravvvv
#11 Screenshot 2023-02-20 at 1.01.56 PM.png63.05 KBgauravvvv
#11 Screenshot 2023-02-20 at 1.03.06 PM.png51.95 KBgauravvvv

Issue fork drupal-3332701

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 CreditAttribution: bspeare commented
Version: 10.0.x-dev » 10.1.x-dev
royalpinto007’s picture

royalpinto007
he/him
Primary language English
CreditAttribution: royalpinto007 commented
Assigned: Unassigned » royalpinto007
Status: Active » Needs work

royalpinto007 opened merge request !3452

royalpinto007’s picture

royalpinto007
he/him
Primary language English
CreditAttribution: royalpinto007 commented
Assigned: royalpinto007 » Unassigned
Status: Needs work » Needs review

Changes done:
- Replaced physical properties (left, right, etc.) with their logical equivalents (inline-start, inline-end, etc.)
- Combined multiple tablesort classes into one

If you have any feedback or questions, please let me know.

I would also like to contribute further by solving additional issues to align with the latest coding standards, if this works.

royalpinto007’s picture

royalpinto007
he/him
Primary language English
CreditAttribution: royalpinto007 commented
Assigned: Unassigned » royalpinto007
Status: Needs review » Needs work
royalpinto007’s picture

royalpinto007
he/him
Primary language English
CreditAttribution: royalpinto007 commented
Status: Needs work » Needs review

Please review the latest commit with the original file.

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

gauravvvv’s picture

gauravvvv CreditAttribution: gauravvvv at Axelerant for Drupal India Association commented

Fixed CCF in #6, and refactored the code.

smustgrave’s picture

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

Since this changes the .css file can we please get some before/after screenshots.

Thanks!

gauravvvv’s picture

gauravvvv CreditAttribution: gauravvvv at Axelerant for Drupal India Association commented
Status: Needs work » Needs review
StatusFileSize
new Screenshot 2023-02-20 at 1.03.06 PM.png51.95 KB
new Screenshot 2023-02-20 at 1.01.56 PM.png63.05 KB

Added before and after patch screenshots. Please review
Before patch:

After patch:

royalpinto007’s picture

royalpinto007
he/him
Primary language English
CreditAttribution: royalpinto007 commented
Assigned: royalpinto007 » Unassigned
smustgrave’s picture

smustgrave CreditAttribution: smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs screenshots

Thanks @Gauravvv

lauriii’s picture

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

Left a comment on the MR. This should be manually tested on RTL too since there's some CSS specific to RTL.

gauravvvv’s picture

gauravvvv CreditAttribution: gauravvvv at Axelerant for Drupal India Association commented
Status: Needs work » Needs review
StatusFileSize
new Screenshot 2023-02-21 at 10.31.34 AM.png97.71 KB

Addressed feedback over MR.
Attached after patch screenshot for RTL. Please review

smustgrave’s picture

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

Thanks!

nod_’s picture

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

The specificity of the selector is changed, there is no need to prefix the rules with .tablesort

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

sourabhjain’s picture

sourabhjain
Primary language English
CreditAttribution: sourabhjain at Valuebound for Valuebound commented
Status: Needs work » Needs review

Removed unused . tablesort class.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ at Eclipse Foundation commented

another question in the MR

smustgrave’s picture

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

Seems background-color was changed back so should be good now.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ at Eclipse Foundation commented

Need someone to test on windows in high contrast mode with a LTR and a RTL language to make sure the changes here work as intended.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ at Eclipse Foundation commented
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs screenshots

With screenshots too

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
CreditAttribution: mherchel as a volunteer commented
Status: Needs review » Needs work
StatusFileSize
new Content___Drupal.png82.76 KB

Need someone to test on windows in high contrast mode with a LTR and a RTL language to make sure the changes here work as intended.

Good catch asking to check this. Because of the specificity of the [dir="rtl], the background image is showing above the mask-image in forced colors mode, making the arrow look kinda weird (as they're superimposed).

I'll push some code shortly.

mherchel’s picture

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

Pushed a fix by flipping the .tablesort element. This also has the benefit of no longer needing the RTL version of the icon.

smustgrave’s picture

smustgrave CreditAttribution: smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs screenshots
StatusFileSize
new Screenshot 2023-04-15 at 7.57.18 PM.png27.83 KB
new Screenshot 2023-04-15 at 7.57.21 PM.png24.16 KB

Uploading some screenshots

1

2

Everything looks good to me.

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.

amber himes matz’s picture

amber himes matz
she/her
Primary language English
Location Portland, OR USA
CreditAttribution: amber himes matz at Drupalize.Me commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

The MR is currently blocked and needs to be rebased. Also, the version on the issue is 11.x-dev but the target branch on the MR is 10.1.x, so maybe the target branch on the MR needs to be updated as well? Just a bit of git-related cleanup required before this can be reviewed by a committer. Thank you!

gauravvvv’s picture

gauravvvv CreditAttribution: gauravvvv at Axelerant for Drupal India Association commented
Status: Needs work » Needs review
StatusFileSize
new 3332701-29.patch9.75 KB

I have added patch for 11.x. Also I have made changes in core/themes/claro/css/components/tables.pcss.css file, as we have renamed the sort-inactive icon. I have updated the name here as well. please review

amber himes matz’s picture

amber himes matz
she/her
Primary language English
Location Portland, OR USA
CreditAttribution: amber himes matz at Drupalize.Me commented

@Gauravvvv Thanks for your attention to the “Needs reroll” tag, but this issue appears to be using an MR-based workflow, not a patch workflow. I realize that the “Needs reroll” tag could be confusing, when what is needed is a rebase on the merge request, not a patch file. Please refer to the docs on rebasing a merge request: https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr...

Also, it does look like there are a number of unresolved threads in the MR that should be marked as resolved if the feedback has been addressed.

smustgrave’s picture

smustgrave CreditAttribution: smustgrave commented
Status: Needs review » Needs work

If the issue can be updated via the MR.

gauravvvv’s picture

gauravvvv CreditAttribution: gauravvvv at Axelerant for Drupal India Association commented

Need to change the target branch, Author can change it to 11.x

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

shmy’s picture

shmy CreditAttribution: shmy commented

As the rebase on 3332701-refactor-claros-tablesort-indicator is broken i did a new one on rebase-testing (sorry for the poor name). Someone who actually worked on the code should review the result. I run the build:css script during every merge conflict resolution and after the final one a lint:css

shmy’s picture

shmy CreditAttribution: shmy commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave CreditAttribution: smustgrave commented
Status: Needs review » Needs work

MR should be updated to point to 11.x or hidden and a new MR for 11.x be placed.

shmy changed the visibility of the branch rebase-testing to hidden.

shmy changed the visibility of the branch 3332701-refactor-claros-tablesort-indicator to hidden.

shmy opened merge request !5864

shweta__sharma’s picture

shweta__sharma CreditAttribution: shweta__sharma at OpenSense Labs commented

Merge request pipeline failed.

shmy’s picture

shmy CreditAttribution: shmy commented
StatusFileSize
new before-tables-sortable-heading-Screenshot_20231219_181436-cropped.png290.81 KB
new after-tables-sortable-heading-Screenshot_20231219_180952-cropped.png290.81 KB
new before-forced-colors-tables-sortable-heading-Screenshot_20231219_181526-cropped.png290.81 KB
new after-forced-colors-tables-sortable-heading-Screenshot_20231219_181302-cropped.png290.81 KB
new before-rtl-wo-is-tables-sortable-heading-Screenshot_20231219_175340-cropped.png290.81 KB
new after-rtl-wo-is-tables-sortable-heading-Screenshot_20231219_174544-cropped.png290.81 KB
new before-rtl-wo-is-forced-colors-tables-sortable-heading-Screenshot_20231219_195647.png7.63 KB
new after-rtl-wo-is-forced-colors-tables-sortable-heading-Screenshot_20231219_195443.png7.63 KB

Here are screenshots for renamed sort--inactive.svg image tables.pcss.css file:

Before (default):
Before (default)
After (default):
After (default)
Before (forced-colors):
Before (forced-colors)
After (forced-colors):
After (forced-colors)

The :is() selector doesn't work for me in neither Firefox or Chromium. I haven't worked with PostCSS before and i don't get why its added there. The following screenshots are made w/o the :is() selector as there is no difference for me when set. I don't know if this is a different issue or i'm making something wrong.

Before (RTL default):
Before (RTL default)
After (RTL default):
After (RTL default)

Before (RTL forced-colors):
Before (RTL forced-colors)
After (RTL forced-colors):
After (RTL forced-colors)

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

meeni_dhobale’s picture

meeni_dhobale CreditAttribution: meeni_dhobale as a volunteer and at QED42 commented
Status: Needs work » Needs review
StatusFileSize
new RTL-Before-refactor-tablesort-indicator.png16.93 KB
new RTL-After-refactor-tablesort-indicator.png26.25 KB
new Before-refactor-tablesort-indicator.png20.29 KB
new After-refactor-tablesort-indicator.png13.49 KB

I removed the @nest deprecated directive and solved the linting errors. Adding before and after screenshots.
RTL-Before

RTL-After

LTR-Before

LTR-After

smustgrave’s picture

smustgrave CreditAttribution: smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs reroll

This looks correct based on the numerous screenshots.

  • nod_ committed 9e0ed313 on 11.x 
    Issue #3332701 by Gauravvvv, shmy, royalpinto007, Meeni_Dhobale,...
nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ at Très Bien Tech commented
Status: Reviewed & tested by the community » Fixed

Committed 9e0ed31 and pushed to 11.x. Thanks!

nod_ closed merge request !5864

Status: Fixed » Closed (fixed)

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