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/claro/css/components/container-inline.module.pcss.css 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
#46 3293997-46.patch1.84 KBakshay kashyap
#42 interdiff-36_42.txt1.05 KBgauravvvv
#42 3293997-42.patch1.21 KBgauravvvv
#41 Screenshot 2023-03-20 at 8.03.15 PM.png27.43 KBsmustgrave
#38 after.png154.75 KBsmustgrave
#38 before.png149.83 KBsmustgrave
#37 Screenshot 2023-03-15 at 4.41.22 PM.png473.09 KBpradipmodh13
#37 Screenshot 2023-03-15 at 4.41.32 PM.png551.39 KBpradipmodh13
#36 interdiff-34_36.txt1.23 KBgauravvvv
#36 3293997-36.patch1.24 KBgauravvvv
#34 3293997-32.patch1.5 KBgauravvvv
#32 interdiff-16_32.txt709 bytesgauravvvv
#32 3293997-32.patch1.36 KBgauravvvv
#31 3293997-nr-bot.txt1.97 KBneeds-review-queue-bot
#16 3293997-version10.0.x.patch1.36 KBaditya4478
#14 3293997-version9.5.x.patch2.18 KBaditya4478
#13 3293997-version10.0.x.patch2.18 KBaditya4478
#12 3293997-10.0.x.patch2.18 KBaditya4478
#11 3293997-10.0.x.patch2.01 KBaditya4478
#10 3293997-9.5.x.patch2.01 KBaditya4478
#8 3293997-9.5.x-version-8.patch2.18 KBMunavijayalakshmi
#5 3293997-9.5.x-version.patch2.01 KBaditya4478
#3 3293997-9.5.x-version.patch1.6 KBaditya4478

Comments

sasanikolic created an issue. See original summary.

sasanikolic’s picture

sasanikolic commented
Issue tags: +Needs followup
aditya4478’s picture

aditya4478 commented
Status: Active » Needs review
StatusFileSize
new 3293997-9.5.x-version.patch1.6 KB
sasanikolic’s picture

sasanikolic commented
Status: Needs review » Needs work 
+++ b/core/themes/claro/css/components/container-inline.module.pcss.css
@@ -3,18 +3,20 @@
+    margin-top: 0.125em;

Let's use a common variable for these margins, since they are repeated multiple times.

aditya4478’s picture

aditya4478 commented
Status: Needs work » Needs review
StatusFileSize
new 3293997-9.5.x-version.patch2.01 KB
sasanikolic’s picture

sasanikolic commented
Status: Needs review » Needs work 
+++ b/core/themes/claro/css/components/container-inline.module.css
@@ -11,17 +11,19 @@
+  --form-items-inline-spacing: 0.125em; /* 2px */

Looking through the coding standards for variable namings here Drupal core CSS coding standards, I believe we should name these variables more strictly to the file that we're using them in.

I suggest something like this, although that might be a bit long: --container-inline-form-items-inline-spacing

Munavijayalakshmi’s picture

Munavijayalakshmi commented
Assigned: Unassigned » Munavijayalakshmi
Munavijayalakshmi’s picture

Munavijayalakshmi commented
StatusFileSize
new 3293997-9.5.x-version-8.patch2.18 KB

changed variable namings as per #6.

Munavijayalakshmi’s picture

Munavijayalakshmi commented
Assigned: Munavijayalakshmi » Unassigned
Status: Needs work » Needs review

test passed.

aditya4478’s picture

aditya4478 commented
StatusFileSize
new 3293997-9.5.x.patch2.01 KB

IE Block is not there. Ready for Review

aditya4478’s picture

aditya4478 commented
Version: 9.5.x-dev » 10.0.x-dev
StatusFileSize
new 3293997-10.0.x.patch2.01 KB

Ignore this patch because it has failed tests. I am uploading new patch for version 10

aditya4478’s picture

aditya4478 commented
StatusFileSize
new 3293997-10.0.x.patch2.18 KB
aditya4478’s picture

aditya4478 commented
StatusFileSize
new 3293997-version10.0.x.patch2.18 KB
aditya4478’s picture

aditya4478 commented
StatusFileSize
new 3293997-version9.5.x.patch2.18 KB
sasanikolic’s picture

sasanikolic commented
Status: Needs review » Needs work

I think we can simplify this even more by using CSS logical properties here by using margin-block-start and margin-block-end for the D10 patch.

aditya4478’s picture

aditya4478 commented
Status: Needs work » Needs review
StatusFileSize
new 3293997-version10.0.x.patch1.36 KB
aditya4478’s picture

aditya4478 commented

Please consider #14 & #16 patches

sasanikolic’s picture

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

Changes look good to me now.

lauriii’s picture

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

Why are the source changes different across 9.5.x and 10.0.x? Shouldn't those be the same for both branches?

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
ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Issue summary: View changes
Issue tags: -Needs followup
ckrina’s picture

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

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

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

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

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

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

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

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Issue summary: View changes
needs-review-queue-bot’s picture

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

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

gauravvvv’s picture

gauravvvv commented
StatusFileSize
new 3293997-32.patch1.36 KB
new interdiff-16_32.txt709 bytes

container-inline.module.css file is not updated, I have attached the path with the updated file. Attached interdiff for same.

gauravvvv’s picture

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

gauravvvv commented
StatusFileSize
new 3293997-32.patch1.5 KB

Ignore patch in #32.

smustgrave’s picture

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

Since the .css file changed think this will require before/after screenshots.

gauravvvv’s picture

gauravvvv commented
StatusFileSize
new 3293997-36.patch1.24 KB
new interdiff-34_36.txt1.23 KB

Added shorthand properties, attached interdiff for same. leaving it as NW, as it still needs after patch screenshot.

pradipmodh13’s picture

pradipmodh13 commented
Status: Needs work » Needs review
StatusFileSize
new Screenshot 2023-03-15 at 4.41.32 PM.png551.39 KB
new Screenshot 2023-03-15 at 4.41.22 PM.png473.09 KB

#36 Patch applied successfully.
For ref attached screenshot.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs screenshots
StatusFileSize
new before.png149.83 KB
new after.png154.75 KB

Testing by using the cd_core test modules.

Attached before/after screenshots of a fieldset. Can see nothing changed.

Before
before

After
after

The last submitted patch, 34: 3293997-32.patch, failed testing. View results

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Reviewed & tested by the community » Needs review

The screenshots in #38 appear to be from Olivero (while this patch only affects Claro).

The CSS looks fine. The only change I would make is to use 2px instead of 0.125em. Core includes the pxToRem PostCSS plugin that automatically converts it to REM. If we specify px, it'd be easier to read and would negate the need for the comment.

Also, where is this being used? The only place I found was in datetime-form.html.twig, and I could find where that was used?

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
StatusFileSize
new Screenshot 2023-03-20 at 8.03.15 PM.png27.43 KB

@mherchel that's my mistake using those claro test modules.

But the template can be found on a node edit form, the author on field.

Here's a screenshot that shows nothing was broken but the PM on the time is cut off. Think if we are updating the css might as well fix that right?

authored on

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review
StatusFileSize
new 3293997-42.patch1.21 KB
new interdiff-36_42.txt1.05 KB

Addressed #40, Made use of px unit on place of em. Attached interdiff for same.

@smustgrave, I can confirm that #41, is happening before patch as well. We can open a follow up issue to address it.

smustgrave’s picture

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

Personally think it would be good to try and fix now but won't hold the ticket up.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 42: 3293997-42.patch, failed testing. View results

akshay kashyap’s picture

akshay kashyap commented

@Team, Could we have created variables for the Margin that we are using in the "container-inline.module.css" file?.

akshay kashyap’s picture

akshay kashyap commented
StatusFileSize
new 3293997-46.patch1.84 KB

Created a new patch. Added margin variables in the "container-inline.module.css" file Please review it.

akshay kashyap’s picture

akshay kashyap commented
Status: Needs work » Needs review
smustgrave’s picture

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

For #46 please include interdiffs and check your patches before uploading.

Remarking RTBC as #42 was previously marked and had a random failure.

The last submitted patch, 42: 3293997-42.patch, failed testing. View results

nod_’s picture

nod_
Primary language French
Location Lille
commented

One question is that we're supposed to have px in the pcss file but rem in the generated css file. That isn't the case here is this expected?

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented

One question is that we're supposed to have px in the pcss file but rem in the generated css file. That isn't the case here is this expected?

Only values 3px or greater get converted to REM (see https://git.drupalcode.org/project/drupal/-/blob/10.1.x/core/scripts/css...). This was added in the initial implementation of PxToRem at https://www.drupal.org/project/drupal/issues/3117698#comment-13857994

nod_’s picture

nod_
Primary language French
Location Lille
commented
Version: 10.0.x-dev » 10.1.x-dev
Status: Reviewed & tested by the community » Fixed

Thanks for the confirmation

Committed e2fde99 and pushed to 10.1.x. Thanks!

  • nod_ committed e2fde991 on 10.1.x 
    Issue #3293997 by Aditya4478, Gauravvvv, Munavijayalakshmi, smustgrave:...

Status: Fixed » Closed (fixed)

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