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

Remaining tasks

Manual testing, see #12.

User interface changes

None. There should be no visual differences.
Please post before/after screenshots and make sure they look the same.

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#42 3332683-nr-bot.txt90 bytesneeds-review-queue-bot
#40 3332683-nr-bot.txt1.62 KBneeds-review-queue-bot
#37 3332683-nr-bot.txt90 bytesneeds-review-queue-bot
#36 Screenshot 2024-03-07 at 3.50.48 pm.png134.84 KBpsebborn
#35 Capture d’écran du 2024-03-05 08-08-08.png34.09 KBnod_
#32 after.png19.44 KBnod_
#32 before.png19.96 KBnod_
#31 Screenshot 2024-02-23 at 6.03.49 PM.png389.62 KBsmustgrave
#19 Screenshot 2023-07-30 at 9.36.03 PM.png82.55 KBlauriii
#13 3332683-after-patch.png294.69 KBpradipmodh13
#13 3332683-before-patch.png289.11 KBpradipmodh13
#5 Screenshot 2023-02-23 at 12.04.32 PM.png314.02 KBgauravvvv
#5 Screenshot 2023-02-23 at 12.47.51 PM.png323.42 KBgauravvvv

Issue fork drupal-3332683

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 !3517

gauravvvv’s picture

gauravvvv commented
Status: Active » Needs review
StatusFileSize
new Screenshot 2023-02-23 at 12.47.51 PM.png323.42 KB
new Screenshot 2023-02-23 at 12.04.32 PM.png314.02 KB

I have added before/after screenshots for reference. please review

Before patch

After patch

smustgrave’s picture

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

Think some more nesting could be done. Especially with the tree classes.

gauravvvv’s picture

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

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

Reran the tests and it seemed to be a random failure.

Nesting looks good

nod_’s picture

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

@nest needs to be removed

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

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

mherchel’s picture

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

I did some additional significant refactoring. This stylesheet is a pain because tabledrag is a pain.

When we test this, we need to be extremely thorough. When testing in Safari (16.1 but also verified in 16.4), I noticed a pre-existing issue where the table rows are overly large. This patch fixes this too.

In regards to the @nest comments above with @nod_, we're having a discussion at https://drupal.slack.com/archives/C1BMUQ9U6/p1681135929045969 on whether we should be using it.

Still setting to NR since it definitely needs additional testing.

pradipmodh13’s picture

pradipmodh13 commented
StatusFileSize
new 3332683-before-patch.png289.11 KB
new 3332683-after-patch.png294.69 KB

Applied Patch. It's working fine.
For ref attached screenshot.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Seems rebase failed to apply.

mherchel’s picture

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

Just merged 10.1.x into the branch, and didn't have any conflicts.

smustgrave’s picture

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

Now it looks good.

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 made their first commit to this issue’s fork.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new Screenshot 2023-07-30 at 9.36.03 PM.png82.55 KB

There's still a regression on Safari where the table rows have a lot of extra padding after row has been moved:

psebborn’s picture

psebborn commented

Picking this up as part of the Claro Contribution day

psebborn’s picture

psebborn commented
Assigned: Unassigned » psebborn
Status: Needs work » Active
psebborn’s picture

psebborn commented
Status: Active » Needs review
Issue tags: +ClaroContributionDay2023

Pushed a fix up for this to the existing MR, ready for review now.

psebborn’s picture

psebborn commented
Assigned: psebborn » Unassigned

psebborn opened merge request !5829

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Thanks

Seems to have an order issue

themes/claro/css/components/tabledrag.css
266:3 ✖ Expected "float" to come before "flex-direction" order/properties-order
themes/claro/css/components/tabledrag.pcss.css
244:5 ✖ Expected "float" to come before "flex-direction" order/properties-order
2 problems (2 errors, 0 warnings)

psebborn’s picture

psebborn commented
Status: Needs work » Needs review

Ah, my mistake. For some reason those errors weren't showing when running `yarn lint:css`. Fixed now and passing checks.

smustgrave’s picture

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

Thanks for fixing that!

quietone’s picture

quietone commented
Issue summary: View changes
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs manual testing

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

When we test this, we need to be extremely thorough.

That is from #12. I see some test results in #19 but no other indication that this change has been thoroughly tested. Therefore, I am tagging for manual testing. I had to restore the standard issue template so I could add testing as a remaining task. Setting to Needs review for testing.

There are two MRs here. Can someone add to the issue summary which one is to be reviewed and hopefully close the other one. Thanks.

shweta__sharma’s picture

shweta__sharma commented

Both the MR has same code the difference is one for against branch 10.1.x ( MR3517 ) and the other one for branch 11.x ( MR5829 )

smustgrave changed the visibility of the branch 3332683-refactor-claros-tabledrag to hidden.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs manual testing
StatusFileSize
new Screenshot 2024-02-23 at 6.03.49 PM.png389.62 KB

So I tested using the Table Drag test module in core.
Altered the route tabledrag_test.test_form to use the claro theme.

Everything appears to be lined up correctly.
I'm able to drag rows around without issue.
Highlighting appears to be the same with/without the MR.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new before.png19.96 KB
new after.png19.44 KB

There is change in the th alignement with this patch see screenshot, suggestion in MR

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review

Addressed #32, Padding-inline-start: 0; was missing on draggable td,th.

smustgrave’s picture

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

Believe I'm seeing the issue reported in #32 has been addressed.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new Capture d’écran du 2024-03-05 08-08-08.png34.09 KB

hierarchical indicator looks broken

psebborn’s picture

psebborn commented
Status: Needs work » Needs review
StatusFileSize
new Screenshot 2024-03-07 at 3.50.48 pm.png134.84 KB

Replicated this issue in Firefox.
Fixed this and pushed it to the MR branch.

Tested in Firefox, Safari, Chrome (on Mac, don't have access to another machine right now)

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3332683-nr-bot.txt90 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 necessarily 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.

Mithun S made their first commit to this issue’s fork.

mithun s’s picture

mithun s
Primary language English
Location Bangalore
commented
Status: Needs work » Needs review

Added a rebase and resolved the merge conflicts. Please review.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3332683-nr-bot.txt1.62 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily 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.

vladimiraus’s picture

vladimiraus
Primary language English
Location Brisbane 🇦🇺
commented
Version: 11.x-dev » 11.0.x-dev
Status: Needs work » Needs review
  • Switched to 11.0.x branch
  • Fixed stylelint
needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3332683-nr-bot.txt90 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 necessarily 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.

quietone’s picture

quietone commented
Version: 11.0.x-dev » 11.x-dev

VladimirAus changed the visibility of the branch 11.x to hidden.

vladimiraus’s picture

vladimiraus
Primary language English
Location Brisbane 🇦🇺
commented
Status: Needs work » Needs review
smustgrave’s picture

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

Rebase seems good.

nod_ closed merge request !5829

  • nod_ committed f1769fae on 11.x 
    Issue #3332683 by VladimirAus, psebborn, Gauravvvv, mherchel, lauriii,...
nod_’s picture

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

Committed f1769fa and pushed to 11.x. Thanks!

Status: Fixed » Closed (fixed)

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