Problem/Motivation

This is part of the CSS modernization initiative.

Steps to reproduce

The stylesheet at https://git.drupalcode.org/project/drupal/-/blob/10.0.x/core/themes/claro/css/components/details.pcss.css needs to be refactored to make use of modern CSS and Drupal core's PostCSS tooling.

Proposed resolution

Use CSS Logical Properties where appropriate
Use CSS nesting where appropriate

Remaining tasks

We need two patches. One for Drupal 9.5.x and one for Drupal 10.0.x
We need a followup issue to refactor this component in Drupal 10.0.x to make use of component-level CSS custom properties and remove IE specific style definitions.

User interface changes

None. There should be no visual differences.

CommentFileSizeAuthor
#37 Screenshot 2023-12-01 141741.png25.36 KBnitin shrivastava
#29 303544-29.patch16.54 KBsantosh_verma
#29 interdiff_24-29.txt1.99 KBsantosh_verma
#29 After.png205.87 KBsantosh_verma
#29 Before.png222.9 KBsantosh_verma
#24 interdiff-12_24.txt5.22 KBgauravvvv
#24 3303544-24.patch16.56 KBgauravvvv
#23 3303544-23.patch11.66 KB_utsavsharma
#23 interdiff_17-23.txt1.06 KB_utsavsharma
#22 interdiff.txt1 KBlauriii
#17 3303544-17.patch12 KBstanzin
#12 3303544-12.patch11.9 KBgauravvvv
#10 3303544-version10.1.x.patch784 bytesstanzin
#5 3303544-5.patch34.67 KBsakthivel m
#3 3303544-version10.0.x.patch10.89 KBaditya4478
#2 3303544-version9.5.x.patch14.98 KBaditya4478

Issue fork drupal-3303544

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

sasanikolic created an issue. See original summary.

aditya4478’s picture

aditya4478 commented
StatusFileSize
new 3303544-version9.5.x.patch14.98 KB
aditya4478’s picture

aditya4478 commented
Version: 9.5.x-dev » 10.0.x-dev
Status: Active » Needs review
StatusFileSize
new 3303544-version10.0.x.patch10.89 KB
sasanikolic’s picture

sasanikolic commented
Status: Needs review » Needs work 
+++ b/core/themes/claro/css/components/details.css
@@ -95,13 +95,66 @@ td .claro-details {
+  [dir="rtl"] & {

Let's not change the RTL classes. This also doesn't compile correctly. 

  1. +++ b/core/themes/claro/css/components/details.css
@@ -95,13 +95,66 @@ td .claro-details {
+  /**

    These comment sections here are weird. 

  2. +++ b/core/themes/claro/css/components/details.css
@@ -126,16 +179,15 @@ td .claro-details {
 .claro-details__summary--accordion-item {
...
-.claro-details--accordion-item:first-child .claro-details__summary--accordion-item {
...
+  .claro-details--accordion-item:first-child & {

    This should be the other way around, the first-child selector should be on the .claro-details--accordion-item with the .claro-details__summary--accordion-item nested.

sakthivel m’s picture

sakthivel m commented
Status: Needs work » Needs review
StatusFileSize
new 3303544-5.patch34.67 KB

#5 Please review the patch

ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Issue tags: +CSS
Parent issue: » #3324398: [META] Update Claro CSS with new coding standards
ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Issue tags: +frontend
ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Issue summary: View changes
smustgrave’s picture

smustgrave commented
Status: Needs review » Postponed
Issue tags: +Needs followup

Per our slack conversation a few weeks ago @ckrina moving these to postponed for I believe a followup on the game plan

stanzin’s picture

stanzin commented
Version: 10.0.x-dev » 10.1.x-dev
Status: Postponed » Needs review
StatusFileSize
new 3303544-version10.1.x.patch784 bytes

This is my first Patch. Just checking for encoding errors.

smustgrave’s picture

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

You can check for build errors by running ./core/scripts/dev/commit-code-check.sh before uploading a patch to make sure there are no issues with code formatting. see https://www.drupal.org/docs/develop/development-tools/running-core-devel...

Also would check how other tickets were done for this as far as updating with logical properties and nesting.

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review
StatusFileSize
new 3303544-12.patch11.9 KB

I have updated the file with the CSS logical properties. I have attached a patch for same. please review

aditya4478’s picture

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

LGTM !

Status: Reviewed & tested by the community » Needs work

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

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Reviewed & tested by the community

Unrelated failure, restoring status.

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.

stanzin’s picture

stanzin commented
StatusFileSize
new 3303544-17.patch12 KB

Patch for V11

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 17: 3303544-17.patch, failed testing. View results

aditya4478’s picture

aditya4478 commented
Status: Needs work » Reviewed & tested by the community

#17 shows unrelated JS failure.

#12 Patch is working for D11. LGTM !

Restoring Status, ready to be merged.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 17: 3303544-17.patch, failed testing. View results

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Reviewed & tested by the community

Patch #12, is still passing all the tests. Restoring status, random failure
@Stanzin, Please add interdiff for #17

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new interdiff.txt1 KB 
  1. +++ b/core/themes/claro/css/components/details.pcss.css
@@ -141,24 +131,19 @@
-[dir="rtl"] .claro-details__summary::before {
...
-  transform: rotate(-270deg);

    This style should be added back

  2. We should move details related variables from variables.pcss.css to details.pcss.css
_utsavsharma’s picture

_utsavsharma commented
StatusFileSize
new interdiff_17-23.txt1.06 KB
new 3303544-23.patch11.66 KB

Tried to address 1 mentioned in #22.

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review
StatusFileSize
new 3303544-24.patch16.56 KB
new interdiff-12_24.txt5.22 KB

Addressed feedback #2, from #22 and feedback #1, from #22, is already addressed in patch #12. I have mentioned in comment #21 that patch #17 is not needed, patch #12 still applies cleanly. I have attached interdiff with #12, for moving detail variables from variable.pcss.css to details.pcss.css

Status: Needs review » Needs work

The last submitted patch, 24: 3303544-24.patch, failed testing. View results

gauravvvv’s picture

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

smustgrave commented
Status: Needs review » Needs work

Indent for new variables in details.pcss.css is off.

Also with the additional changes could use a new set of before/after screenshots

Thanks.

santosh_verma’s picture

santosh_verma commented

working on it

santosh_verma’s picture

santosh_verma commented
Status: Needs work » Needs review
StatusFileSize
new Before.png222.9 KB
new After.png205.87 KB
new interdiff_24-29.txt1.99 KB
new 303544-29.patch16.54 KB

In this Patch addressing the comment #27,
attaching interdiff file.

Testing stepps
1.Install drupal 11.x.
2.navigate to /admin/structure/types/add

Before
Before
After
After

smustgrave’s picture

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 29: 303544-29.patch, failed testing. View results

smustgrave’s picture

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

Appears to be random

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

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

vsujeetkumar opened merge request !5567

vsujeetkumar’s picture

vsujeetkumar commented
Status: Needs work » Needs review

Convert Patch #29 into MR. According to #33. Please have a look.

nitin shrivastava’s picture

nitin shrivastava commented
StatusFileSize
new Screenshot 2023-12-01 141741.png25.36 KB

@vsujeetkumar MR#36 looks fine and applied successfully. Just a simple reroll into MR Moving this to RTBC.
Thanks !

nitin shrivastava’s picture

nitin shrivastava commented
nitin shrivastava’s picture

nitin shrivastava commented
Status: Needs review » Reviewed & tested by the community
finnsky’s picture

finnsky commented
Status: Reviewed & tested by the community » Needs work

Gonna check nightwatch failure

finnsky’s picture

finnsky commented
Status: Needs work » Needs review

Nightwatch issue is gone after rebase. It was something random.

I reverted variables back to variables file. Because at least half of them used in few files. And removed one which not used anywhere.

Added few minor improvements.
Please review.

smustgrave’s picture

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

Gotta love night watch haha

  • ckrina committed d8e3d8c9 on 11.x 
    Issue #3303544 by Gauravvvv, Aditya4478, finnsky, Stanzin, vsujeetkumar...
ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Status: Reviewed & tested by the community » Fixed

Committed d8e3d8c and pushed to 11.x. Thanks!

Status: Fixed » Closed (fixed)

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