Problem/Motivation

This is part of the CSS modernization initiative. This is intended to be a straightforward second issue.

The first issue was regarding the container-inline stylesheet.

Steps to reproduce

The stylesheet at https://git.drupalcode.org/project/drupal/-/blob/10.0.x/core/themes/claro/css/components/container-inline.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.

User interface changes

None. There should be no visual differences.

CommentFileSizeAuthor
#28 3293998-28.patch1.21 KBgauravvvv
#27 3293998-27.patch578.33 KBstanzin
#23 interdiff_21-23.txt622 bytesRatan Priya
#23 3293998-23.patch1.27 KBRatan Priya
#21 3293998-10.0.x.patch1.45 KBaditya4478
#19 3293998.patch1.45 KBaditya4478
#17 3293998-9.5.x.patch1.32 KBaditya4478
#15 3293998-Version9.5.x.patch1.41 KBaditya4478
#14 3293998-Version9.5.x.patch0 bytesaditya4478
#11 3293998-version10.0.x.patch1.09 KBaditya4478
#10 3293998-version10.0.x.patch1.09 KBaditya4478
#9 3293998-version9.5.x.patch1.25 KBaditya4478
#8 3293998-version9.5.x.patch1.28 KBaditya4478
#6 3293998-10.0.x.patch1.5 KBaditya4478
#5 3293998-10.0.x.patch3.51 KBaditya4478
#4 3293998-9.5.x.patch2.91 KBaditya4478
#2 3293998-9.5.x-version.patch1.5 KBaditya4478

Issue fork drupal-3293998

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
Status: Active » Needs review
StatusFileSize
new 3293998-9.5.x-version.patch1.5 KB
sasanikolic’s picture

sasanikolic commented
Status: Needs review » Needs work 
+++ b/core/themes/claro/css/components/container-inline.pcss.css
@@ -3,13 +3,17 @@
+  [dir="rtl"] & .form-type-radio {

Let's leave this out of the nesting for now.

For the Drupal 10 patch, we can then use CSS logical properties. See https://css-tricks.com/css-logical-properties/ for examples and comments in Styling for Right-To-Left Languages

aditya4478’s picture

aditya4478 commented
Status: Needs work » Needs review
StatusFileSize
new 3293998-9.5.x.patch2.91 KB

Instructions noted & changes done as per comment #3
No Blocks of IE are there. Ready for review

aditya4478’s picture

aditya4478 commented
Version: 9.5.x-dev » 10.0.x-dev
StatusFileSize
new 3293998-10.0.x.patch3.51 KB

I will post new patch to make test pass for version 10

aditya4478’s picture

aditya4478 commented
StatusFileSize
new 3293998-10.0.x.patch1.5 KB
sasanikolic’s picture

sasanikolic commented
Status: Needs review » Needs work 
+++ b/core/themes/claro/css/components/container-inline.module.css
@@ -10,18 +10,20 @@
+  &div,

Something's wrong here, we should not have & in the compiled css and nesting.

aditya4478’s picture

aditya4478 commented
Version: 10.0.x-dev » 9.5.x-dev
Status: Needs work » Needs review
StatusFileSize
new 3293998-version9.5.x.patch1.28 KB

ignore this patch

aditya4478’s picture

aditya4478 commented
StatusFileSize
new 3293998-version9.5.x.patch1.25 KB
aditya4478’s picture

aditya4478 commented
Version: 9.5.x-dev » 10.0.x-dev
StatusFileSize
new 3293998-version10.0.x.patch1.09 KB

ignore this

aditya4478’s picture

aditya4478 commented
StatusFileSize
new 3293998-version10.0.x.patch1.09 KB
aditya4478’s picture

aditya4478 commented

Consider #9 & #11 Patches

sasanikolic’s picture

sasanikolic commented
Status: Needs review » Needs work 
+++ b/core/themes/claro/css/components/container-inline.pcss.css
@@ -3,11 +3,13 @@
+  & label:after {

Nit: let's use :: for pseudo-elements like before and after.

aditya4478’s picture

aditya4478 commented
Version: 10.0.x-dev » 9.5.x-dev
Status: Needs work » Needs review
StatusFileSize
new 3293998-Version9.5.x.patch0 bytes

Ignore this patch, I have made mistake in git commands while making this patch

aditya4478’s picture

aditya4478 commented
StatusFileSize
new 3293998-Version9.5.x.patch1.41 KB

Consider this patch. #13 comment remembered & improved.

sasanikolic’s picture

sasanikolic commented
Status: Needs review » Needs work 
+++ b/core/themes/claro/css/components/container-inline.pcss.css
@@ -3,11 +3,13 @@
+  .container-inline .form-radios .form-type-radio {

This is not correct. This nested selector should just be .form-type-radio.

aditya4478’s picture

aditya4478 commented
Status: Needs work » Needs review
StatusFileSize
new 3293998-9.5.x.patch1.32 KB

Noted

sasanikolic’s picture

sasanikolic commented
Status: Needs review » Needs work

We also need the patch for Drupal 10 updated. :)

aditya4478’s picture

aditya4478 commented
Version: 9.5.x-dev » 10.0.x-dev
Status: Needs work » Needs review
StatusFileSize
new 3293998.patch1.45 KB
sasanikolic’s picture

sasanikolic commented
Status: Needs review » Needs work 
+++ b/core/themes/claro/css/components/container-inline.pcss.css
@@ -3,13 +3,11 @@
+    margin-inline-end: 1em;

This should be margin-inline-start.

Once that is fixed, should be ready to go.

aditya4478’s picture

aditya4478 commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new 3293998-10.0.x.patch1.45 KB
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

The patch for 10.x needs to have "yarn run build:css" run again then the resulting patch uploaded. We needed to land #3312481: Update core's browserlist before this patch was really possible.

I don't think we should be changing 9.5.x - can't really see the value.

+++ b/core/themes/claro/css/components/container-inline.pcss.css
@@ -3,13 +3,11 @@
-.container-inline .form-radios label:after {
-  content: "";
-}
-.container-inline .form-radios .form-type-radio {
-  margin-right: 1em;
-}
-[dir="rtl"] .container-inline .form-radios .form-type-radio {
-  margin-right: 0;
-  margin-left: 1em;
+.container-inline .form-radios {
+  & label::after {
+    content: "";
+  }
+  & .form-type-radio {
+    margin-inline-end: 1em;
+  }

Why not change this to:

.container-inline .form-radios label:after {
  content: "";
}
.container-inline .form-radios .form-type-radio {
  margin-inline-end: 1em;
}

Then the indentation in the resulting css wouldn't be borked. I'm not sure I understand what's better with the & format.

Ratan Priya’s picture

Ratan Priya commented
Status: Needs work » Needs review
StatusFileSize
new 3293998-23.patch1.27 KB
new interdiff_21-23.txt622 bytes

@alexpott,
Made changes as per comment #22.
Needs review.

ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Parent issue: » #3324398: [META] Update Claro CSS with new coding standards
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

Believe this is postponed via our conversation a few weeks ago @ckrina. For the followup and a path forward

stanzin’s picture

stanzin commented
Version: 10.0.x-dev » 11.x-dev
Status: Postponed » Needs review
StatusFileSize
new 3293998-27.patch578.33 KB

I think this patch would work.

gauravvvv’s picture

gauravvvv commented
StatusFileSize
new 3293998-28.patch1.21 KB

Patch #23, does not apply anymore and patch #27 includes so many files which are out of scope. I have added patch for 11.x-dev. please review

aditya4478’s picture

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

LGTM !

I will remind @stanzin about the patch out of scope. 👍

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 28: 3293998-28.patch, failed testing. View results

gauravvvv’s picture

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

Random failure, restoring status

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 28: 3293998-28.patch, failed testing. View results

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Reviewed & tested by the community
Issue tags: +Random test failure
ahmad smhan’s picture

ahmad smhan commented

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

  • lauriii committed fa8db8e8 on 11.x 
    Issue #3293998 by Aditya4478, Gauravvvv, sasanikolic, alexpott: Refactor...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs followup, -Random test failure

Committed fa8db8e and pushed to 11.x. Thanks!

Status: Fixed » Closed (fixed)

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