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/card.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
#17 3303543-10.patch3.72 KBzeeshan_khan
#11 Screenshot 2023-06-07 at 2.35.15 PM.png244.77 KBsantosh_verma
#9 3303543-9.patch3.72 KBgauravvvv
#3 3303543-version10.0.x.patch3.95 KBaditya4478
#2 3303543-version9.5.x.patch3.84 KBaditya4478

Issue fork drupal-3303543

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 3303543-version9.5.x.patch3.84 KB
aditya4478’s picture

aditya4478 commented
Version: 9.5.x-dev » 10.0.x-dev
StatusFileSize
new 3303543-version10.0.x.patch3.95 KB
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

aditya4478’s picture

aditya4478 commented
Status: Postponed » Reviewed & tested by the community

@smustgrave I think, it should be comitted on github. Can you say why it was postponed please ?

aditya4478’s picture

aditya4478 commented
Version: 10.0.x-dev » 11.x-dev
Status: Reviewed & tested by the community » Needs work

patch needed for 11.x

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review
StatusFileSize
new 3303543-9.patch3.72 KB

I have updated all the logical properties, attached patch for 11.x-dev. please review

santosh_verma’s picture

santosh_verma commented

doing review of patch #9

santosh_verma’s picture

santosh_verma commented
StatusFileSize
new Screenshot 2023-06-07 at 2.35.15 PM.png244.77 KB

I have tested patch #9, and it applied cleanly without any issues. The nesting and logical properties of the code changes appear to good to me. Nothing is appearing broken, for reference attaching the screenshot.
after

RTBC + 1

zeeshan_khan’s picture

zeeshan_khan commented

@Gauravvvv - I just want clarification in this code block for my better understanding. why didn't you use padding-inline shrothand instead of padding-inline-start as I see padding-right and padding-left both are present.
Rest everything seems good to me Thanks

  /* Card content with image. */ 
   .card--horizontal .card__image ~ .card__content-wrapper {
-    padding-left: var(--space-m);
-  }
-  [dir="rtl"] .card--horizontal .card__image ~ .card__content-wrapper {
-    padding-right: var(--space-m);
-    padding-left: var(--space-l);
+    padding-inline-start: var(--space-m);
   }
 }
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs followup

Follow up is no longer needed and screenshots added in #11

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: 3303543-9.patch, failed testing. View results

gauravvvv’s picture

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

Unrelated failure, restoring status.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 9: 3303543-9.patch, failed testing. View results

zeeshan_khan’s picture

zeeshan_khan commented
Status: Needs work » Needs review
StatusFileSize
new 3303543-10.patch3.72 KB
smustgrave’s picture

smustgrave commented

@zeeshan_khan can you provide an interdiff of what’s different from #9. From what I can tell #9 still applies cleanly to 11.x and failure was probably unrelated.

zeeshan_khan’s picture

zeeshan_khan commented

@smustgrave I am just checking with latest rebase why it was failing and getting back to Needs work. Because #9 looks absolutely fine and works perfectly.

zeeshan_khan’s picture

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

Looks like #9 has also passed the PHP test now. Moving back to RTBC.

smustgrave’s picture

smustgrave commented

Since #17 was reupload going to hide that one.

  • nod_ committed 6985e23b on 11.x 
    Issue #3303543 by Aditya4478, Gauravvvv: Refactor Claro's card...

  • nod_ committed abd9e944 on 10.2.x 
    Issue #3303543 by Aditya4478, Gauravvvv: Refactor Claro's card...
nod_’s picture

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

Committed 6985e23 and pushed to 11.x. Thanks!

Status: Fixed » Closed (fixed)

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

zeeshan_khan opened merge request !6901