Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.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.
| Comment | File | Size | Author |
|---|---|---|---|
| #75 | 3332446-test(3).png | 220.86 KB | nayana_mvr |
| #75 | 3332446-test(2).png | 1.54 MB | nayana_mvr |
| #75 | 3332446-test(1).png | 1.35 MB | nayana_mvr |
| #57 | after.png | 236.81 KB | sourojeetpaul |
| #57 | before.png | 236.81 KB | sourojeetpaul |
Issue fork drupal-3332446
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:
changes
Comments
Comment #2
bspeare commentedComment #5
gauravvvv commentedComment #6
smustgrave commentedAs the .css changes so much definitely think this needs before/after screenshots.
Comment #7
gauravvvv commentedI have added before/patch screenshot.
Before
After
Comment #8
smustgrave commentedThanks!
Comment #9
nod_few pixel offset in LTR for the checkboxes and the description, new patch is more consistent between LTR and RTL so not an issue.
Question about unit mixing in a single rule on the MR.
Comment #11
sourabhjainResolved the issue mentioned in #9
Comment #12
nod_should have been more detailed in the feedback :p
Comment #13
gauravvvv commentedLeft a comment on MR @nod_
Comment #14
gauravvvv commentedComment #15
smustgrave commented@Gauravvvv #13 you say you left a comment but don't see it?
Comment #16
santosh_verma commentedworking on it to address comment #12
Comment #17
santosh_verma commentedComment #12 addressed in this current patch.
Rem replaced with PX as suggest in the comment #12
Comment #18
smustgrave commentedThis ticket was started in an MR and should be continued there. Especially since no interdiff was provided
@Gauravvvv what was the comment you mentioned?
Comment #20
_pratik_Attached interdiff between last two commits,
Thanks
Comment #21
santosh_verma commented@smustgrave Pcss.css not generating (0 2px 4px rgba(0, 0, 0, 0.1) into rem after compilation. I tasted in my local the output was 0 2px 0.25rem rgba(0, 0, 0, 0.1);.
you can also refer the commit #b3156b80
In the commit #75ef2986 PX converted with rem in the Pcss.css file and in my suggestion we can go with that.
Comment #22
gauravvvv commentedEarlier it was same
--module-table-filter-box-shadow: 0 0.125rem 0.25rem rgba(0, 0, 0, 0.1);and postcss converts itinto --module-table-filter-box-shadow: 0 2px 0.25rem rgba(0, 0, 0, 0.1);. That's why I have used rem to avoid this.Reason behind this is, any value below 4px is not compiling to px from rem unit.
Comment #23
smustgrave commentedSounds good to me.
Comment #24
santosh_verma commented@smustgrave your last comment #23 is response to #22 ? but you changed the status need review to need work if any change needed please mention so that we can work on this :)
I tested the #22 MR and working fine
Before
After
Comment #25
gauravvvv commented@smustgrove, you have updated the status to needs work and commented.
Is the status update intentional or by mistake?
Comment #26
smustgrave commentedComment #27
needs-review-queue-bot commentedThe 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 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.
Comment #28
gauravvvv commentedComment #29
smustgrave commentedLooks good!
Comment #31
needs-review-queue-bot commentedThe 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.
Comment #32
meeni_dhobale commentedWorking on this issue as a part of Claro Contribution Day.
Comment #33
meeni_dhobale commentedComment #34
meeni_dhobale commentedI refactor the .pcss.css file. I was trying to rebase the current branch but facing some issues, so adding patch for now.
Here is the previous module list file.
Here are the screenshots of the changes.
Comment #35
meeni_dhobale commentedComment #36
smustgrave commentedWe shouldn't revert back to the old way of doing things.
Issue might be the current MR is pointed at 10.1.x we need one for 11.x
Comment #39
gauravvvv commentedI have updated leftover CSS logical properties. please review
Comment #40
shweta__sharma commentedTested MR 5830 and it is applied cleanly now stylesheet using logical properties and nesting CSS.
Attached after and before screenshots
Can we move this to RTBC now?
Comment #41
shweta__sharma commentedComment #42
smustgrave commentedIf you feel you've done a full code review, nothing has broken, and the core gates have been met then you can RTBC it https://www.drupal.org/about/core/policies/core-change-policies/core-gates
Comment #43
shweta__sharma commentedThanks @smustgrave for the guidance.
Moving this to RTBC LGTM
Comment #44
nod_Comment #46
mithun sAddressed the review comments and updated the PR. Changing the status to Needs review.
Comment #47
nod_few details remaining
Comment #48
mithun sAddressed all the review comments and pushed a commit. Changing the status to Needs review.
Thank you!
Comment #49
smustgrave commentedappears feedback has been addressed
Comment #50
nod_regresson on the "show all column" button when viewing on the mobile view.
Comment #52
sandeepmahlawat commentedi have implemented the solution as prescribed please review.
Comment #53
needs-review-queue-bot commentedThe 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.
Comment #55
sourojeetpaul commentedComment #56
smustgrave commentedSeems to have a test failure, may need a rebase but should be tested.
Comment #57
sourojeetpaul commented@smustgrave, As far as I've tested nothing seems to be breaking! Attaching before and screenshits for reference.
Also seems like PhpUnit test is failing over here, but not sure why PhpUnit tests are failing when the MR only make chnages on PCSS and CSS files.
Comment #58
sourojeetpaul commentedComment #59
psebborn commentedI've rebased from 11.x which seems to has resolved the pipeline. Ready for review
Comment #60
smustgrave commentedLeft comments on MR.
Comment #61
mithun sComment #62
mithun sComment #63
mithun sComment #64
smustgrave commentedSmall comment.
Comment #66
sanket.tale commentedComment #67
smustgrave commentedSeemed to generate wrong see .css file
Comment #68
sanket.tale commentedComment #69
smustgrave commentedFeedback appears to be adderessed.
Comment #70
nod_There is a couple of px of difference in RTL (3px difference on the checkbox position), not a big deal but would be nice to find why.
Couple of comments left.
I've given the same feedback twice in a couple of places. Please make sure feedback is kept when moving branches or trying to restart work somewhere.
Comment #71
sanket.tale commentedComment #73
smustgrave commentedSmall nitpicky comments on the comments.
Comment #74
psebborn commentedI've actioned the above points (added back the comment) - see MR for an explanation of why I've placed it inside the selector.
Comment #75
nayana_mvr commentedPHPUnit Functional Javascript failures shown in the MR are not reproducible in local. Command used
vendor/bin/phpunit --configuration phpunit.xml {filename}. Attaching screenshots as evidence. Please let me know if there is any other way to reproduce it in local. Moving this ticket back to Needs Work so that someone else can also verify it once.