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/fieldset.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
#15 3303548-15.patch6.67 KBStanzin
#12 Screenshot 2023-06-29 at 10.12.39 AM.png239.65 KBGauravvvv
#10 interdiff-8_10.txt3.25 KBGauravvvv
#10 3303548-10.patch6.68 KBGauravvvv
#8 3303548-8.patch7.52 KBStanzin
#3 3303548-version10.0.x.patch7.97 KBAditya4478
#2 3303548-version9.5.x.patch8.59 KBAditya4478
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sasanikolic created an issue. See original summary.

Aditya4478’s picture

Aditya4478 CreditAttribution: Aditya4478 commented
Status: Active » Needs review
FileSize
3303548-version9.5.x.patch8.59 KB
Aditya4478’s picture

Aditya4478 CreditAttribution: Aditya4478 commented
Version: 9.5.x-dev » 10.0.x-dev
FileSize
3303548-version10.0.x.patch7.97 KB
ckrina’s picture

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

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

smustgrave CreditAttribution: smustgrave at Mobomo 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 CreditAttribution: Aditya4478 commented
Status: Postponed » Needs work
Stanzin’s picture

Stanzin CreditAttribution: Stanzin as a volunteer commented
Version: 10.0.x-dev » 11.x-dev
Status: Needs work » Needs review
FileSize
3303548-8.patch7.52 KB
smustgrave’s picture

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

Can we get before/after screenshots added to the IS please.

Gauravvvv’s picture

Gauravvvv CreditAttribution: Gauravvvv at Axelerant for Drupal India Association commented
Status: Needs work » Needs review
FileSize
3303548-10.patch6.68 KB
interdiff-8_10.txt3.25 KB

I have fixed the selectors and short hand properties. Attached interdiff for same. So many wrong selectors were used in patch #8

Exp: +--composite.fieldset__legend {

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Needs work

Can we get screenshots please.

Gauravvvv’s picture

Gauravvvv CreditAttribution: Gauravvvv at Axelerant for Drupal India Association commented
Status: Needs work » Needs review
Issue tags: -Needs screenshots
FileSize
Screenshot 2023-06-29 at 10.12.39 AM.png239.65 KB

I have added before and after patch screenshot.

smustgrave’s picture

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

Thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 10: 3303548-10.patch, failed testing. View results

Stanzin’s picture

Stanzin CreditAttribution: Stanzin as a volunteer commented
Status: Needs work » Needs review
FileSize
3303548-15.patch6.67 KB

Re-Rolling patch. I think #10 patch is failed due to unrelated failure.

Aditya4478’s picture

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

LGTM !

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 15: 3303548-15.patch, failed testing. View results

smustgrave’s picture

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

Seems random failure.

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented

I'm triaging RTBC issues. I read the IS and the comments. I didn't find any unanswered questions but the proposed resolution is out of date.

Leaving at RTBC.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 15: 3303548-15.patch, failed testing. View results

shweta__sharma’s picture

shweta__sharma CreditAttribution: shweta__sharma at OpenSense Labs commented
Status: Needs work » Reviewed & tested by the community

Seems random failure.

kostyashupenko’s picture

kostyashupenko
Primary language Russian
Location Omsk
CreditAttribution: kostyashupenko at Skilld commented

  • nod_ committed f0ef38ff on 11.x 
    Issue #3303548 by Aditya4478, Stanzin, Gauravvvv: Refactor Claro's...

  • nod_ committed 59050575 on 10.2.x 
    Issue #3303548 by Aditya4478, Stanzin, Gauravvvv: Refactor Claro's...
nod_’s picture

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

Committed f0ef38f and pushed to 11.x. Thanks!

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented

There was a small regression to the fieldset styles. I opened separate issue for addressing that: #3396738: Regression in fieldset legend positioning. 😊

Status: Fixed » Closed (fixed)

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