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/form--managed-file.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
#32 3303550-32.patch13.44 KBstanzin
#12 3303550-12.patch7.64 KB_utsavsharma
#5 3303550-version9.5.x.patch8.23 KBaditya4478
#3 3303550.version10.0.x.patch17.03 KBaditya4478
#2 3303550-version9.5.x.patch12.52 KBaditya4478

Issue fork drupal-3303550

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 3303550-version9.5.x.patch12.52 KB
aditya4478’s picture

aditya4478 commented
Version: 9.5.x-dev » 10.0.x-dev
StatusFileSize
new 3303550.version10.0.x.patch17.03 KB
sasanikolic’s picture

sasanikolic commented
Version: 10.0.x-dev » 9.5.x-dev
Status: Needs review » Needs work 
  1. +++ b/core/themes/claro/css/components/form--managed-file.pcss.css
@@ -39,19 +40,19 @@
+  .draggable .form-managed-file.has-value & {

    & should be at the beginning of the line, no? 

  2. +++ b/core/themes/claro/css/components/form--managed-file.pcss.css
@@ -39,19 +40,19 @@
+  /* Add some bottom margin for single widgets if no meta is present. */
+  .form-managed-file.is-single.has-value &:last-child {

    I don't think this will work when nested like this? Check the compiled selectors. 

  3. +++ b/core/themes/claro/css/components/form--managed-file.pcss.css
@@ -73,14 +74,14 @@
+  .draggable .form-managed-file.has-value & {

    Same here, & at the start of the line.

aditya4478’s picture

aditya4478 commented
Status: Needs work » Needs review
StatusFileSize
new 3303550-version9.5.x.patch8.23 KB

Please ignore above patches, and review this for D9.

smustgrave’s picture

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

Reviewed patch
Applies cleanly
Tested against an file and image field and nothing seemed to break.

lauriii’s picture

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

This needs a Drupal 10 patch too.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

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 » Needs work

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 [1] as a guide.

Unfortunately the 10.0 window has based and tried running 10.1 tests for #3
but appears to have some errors.

Also there is a follow up needed

_utsavsharma’s picture

_utsavsharma commented
Status: Needs work » Needs review
StatusFileSize
new 3303550-12.patch7.64 KB

Fixed patch #5 as per by #7.
Please review.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Still needs the follow up. Please read the tags before rerolling.
Also attach interdiffs for the changes

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

Gauravvv opened merge request !3571

gauravvvv’s picture

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

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

.form-managed-file__main has 2 sections that could be merged
form-managed-file__meta has 2 sections that could be merged.

Anonymous’s picture

Anonymous (not verified) commented

Gaurav-drupal made their first commit to this issue’s fork.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs work » Needs review

Addressed #17, please review

smustgrave’s picture

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

Nesting seems much better.

nod_’s picture

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

agreed, nesting is nice here. Simply need to remove @nest

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

gauravvvv’s picture

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

Restoring status as we are going ahead with @nest as of now.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs followup

You can remove the line and set back to RTBC, appart from that the patch is good to go.

gauravvvv’s picture

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

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

Moving to RBTC per #24

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

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Status: Reviewed & tested by the community » Needs work

Unaddressed feedback from @nod_, and I added some additional items in the MR. @nod_ is correct that The @nest syntax (literally the thing that says "@nest") should not be used, but you can still nest by placing the ampersand after the selector.

@VladimirAus - I've mentioned this in other issues, but can you refrain from clicking the rebase button if you're not making additional contributions. It makes the issue harder to follow, and anyone making changes to the branch can get things current with HEAD with about 5 seconds work so it doesn't really help advance anything.

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review

I have addressed all the feedbacks. please review

smustgrave’s picture

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

@nod_ correct me if I'm wrong the no @nest thing was when it was discussed moving away from that but that's no longer the case

Points do appear to be addressed.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

stanzin’s picture

stanzin commented
StatusFileSize
new 3303550-32.patch13.44 KB

Rerolling Patch for V11 :)

Status: Reviewed & tested by the community » Needs work

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

gauravvvv’s picture

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

MR 3571, still applies to D11. There is no need of re-roll. thanks

restoring status.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Status: Reviewed & tested by the community » Needs work

I left a few bits of feedback on the MR. They should be pretty easy to address though.

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

rpayanm’s picture

rpayanm
he/him
Primary language Spanish
commented
Status: Needs work » Needs review

Please review.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

For the open threads

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review

Addressed all threads, please review

smustgrave’s picture

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

Threads appear to be addressed.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Hiding all the files because we have MRs

  • lauriii committed 86a0d306 on 11.x 
    Issue #3303550 by Gauravvvv, Aditya4478, rpayanm, bnjmnm, _utsavsharma,...

lauriii closed merge request !3571

lauriii’s picture

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

Committed 86a0d30 and pushed to 11.x. Thanks!

Status: Fixed » Closed (fixed)

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