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.

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.
issue image

CommentFileSizeAuthor
#12 3305148 issue image.png103.31 KBHarish1688
#10 3305148-8.patch3.47 KBgauravvvv
#9 refactor-image-preview-3305148-9.patch1.81 KBHarish1688
#8 refactor-image-preview-3305148-8.patch1.56 KBHarish1688
#8 interdiff_3-8.txt3.17 KBHarish1688
#8 3305148 after patch .png71.15 KBHarish1688
#3 3305148-version10.0.x.patch3.45 KBaditya4478
#2 3305148-version9.5.x.patch3.18 KBaditya4478

Comments

Aditya4478 created an issue. See original summary.

aditya4478’s picture

aditya4478 commented
Status: Active » Needs review
StatusFileSize
new 3305148-version9.5.x.patch3.18 KB
aditya4478’s picture

aditya4478 commented
Version: 9.5.x-dev » 10.0.x-dev
StatusFileSize
new 3305148-version10.0.x.patch3.45 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
Issue tags: +Needs Review Queue Initiative

This issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 200, following Review a patch or merge require as a guide.

Was discussed in slack #frontend channel slightly with @ckrina and @quietone
Moving to postponed as there are still some decisions to be made about how to go about these changes and what's needed in the follow up.

aditya4478’s picture

aditya4478 commented
Status: Postponed » Needs work
Harish1688’s picture

Harish1688 commented
Status: Needs work » Needs review
StatusFileSize
new 3305148 after patch .png71.15 KB
new interdiff_3-8.txt3.17 KB
new refactor-image-preview-3305148-8.patch1.56 KB

Hi,

Tested the #3 patch (305148-version10.0.x.patch) but it's failed to apply, re-roll the patch for 10 and 11 version.

1. CSS Logical Properties and nesting are utilized appropriately.
2. After patch UI showing no difference in compared to the previous version.

Screenshot attached

Need Review

Harish1688’s picture

Harish1688 commented
StatusFileSize
new refactor-image-preview-3305148-9.patch1.81 KB

Hi ,

CSS compile Error #8, resolve in the patch (refactor-image-preview-3305148-9.patch), after compile the file, no modification show in 'image-preview.css'. However it's not in the patch.

Needs Review.

gauravvvv’s picture

gauravvvv commented
StatusFileSize
new 3305148-8.patch3.47 KB

I have refactored the image-preview stylesheet, Attached patch for same.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: -Needs followup +Needs screenshots

Before/after screenshots should be included in the issue summary

Thanks.

Harish1688’s picture

Harish1688 commented
Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new 3305148 issue image.png103.31 KB

Hi,

As per #11 request attached the image in issue summary.
The UI should remain visually consistent before/after the patch applied, with no need to compare the screens before and after screen.

smustgrave’s picture

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

Nesting appears good and will lean on screenshot #12

  • lauriii committed e45ee776 on 11.x 
    Issue #3305148 by Harish1688, Aditya4478, Gauravvvv, smustgrave:...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Version: 10.0.x-dev » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed e45ee77 and pushed to 11.x. Thanks!

Status: Fixed » Closed (fixed)

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