Problem/Motivation

This is part of the CSS modernization initiative.

The first issue was regarding the form--text stylesheet.

Steps to reproduce

The stylesheet at https://git.drupalcode.org/project/drupal/-/blob/10.0.x/core/themes/claro/css/components/form--text.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.

User interface changes

None. There should be no visual differences.

CommentFileSizeAuthor
#37 interdiff-33_37.txt1.26 KBgauravvvv
#37 3294006-37.patch1.17 KBgauravvvv
#34 3294006 after patch.png9.12 KBHarish1688
#33 interdiff-29_33.txt1.17 KBgauravvvv
#33 3294006-33.patch1.92 KBgauravvvv
#29 3294006-29.patch1.26 KB_utsavsharma
#26 reroll_diff_24-26.txt7.23 KBmedha kumari
#26 3294006.9.5.x-26.patch637 bytesmedha kumari
#24 reroll_diff_3294006.txt2.66 KBsakthivel m
#24 3294006.9.5.x-24.patch6.45 KBsakthivel m
#22 3294006.9.5.x.patch6.02 KBaditya4478
#20 3294006-version10.0.x.patch10.21 KBaditya4478
#19 3294006-version9.5.x.patch5.67 KBaditya4478
#18 3294006-version9.5.x.patch2.49 KBaditya4478
#17 3294006-9.5.x.patch5.31 KBaditya4478
#16 3294006-10.0.x.patch6.36 KBaditya4478
#14 3294006.9.5.x.patch6.81 KBaditya4478
#12 3294006-10.0.x-12.patch17.03 KBshashwat-tiwari
#6 interdiff-4-6.txt946 bytesshashwat-tiwari
#6 3294006-9.5.x-6.patch5.74 KBshashwat-tiwari
#4 3294006-9.5.x-4.patch5.74 KBMunavijayalakshmi
#2 3294006-9.5.x.patch5.81 KBaditya4478

Comments

sasanikolic created an issue. See original summary.

aditya4478’s picture

aditya4478 commented
Status: Active » Needs review
StatusFileSize
new 3294006-9.5.x.patch5.81 KB
Munavijayalakshmi’s picture

Munavijayalakshmi commented
Assigned: Unassigned » Munavijayalakshmi
Munavijayalakshmi’s picture

Munavijayalakshmi commented
StatusFileSize
new 3294006-9.5.x-4.patch5.74 KB

Rectified custom command line errors.

themes/claro/css/components/form--text.pcss.css
146:5 ✖ Expected no more than 0 empty lines selector-max-empty-lines
153:1 ✖ Unexpected empty line before closing brace block-closing-brace-empty-line-before

Munavijayalakshmi’s picture

Munavijayalakshmi commented
Assigned: Munavijayalakshmi » Unassigned
shashwat-tiwari’s picture

shashwat-tiwari commented
StatusFileSize
new 3294006-9.5.x-6.patch5.74 KB
new interdiff-4-6.txt946 bytes

Updated the patch for 9.5.x.

shashwat-tiwari’s picture

shashwat-tiwari commented
Issue tags: +Needs reroll

Patch needs to be rerolled for 10.0.x branch.

Munavijayalakshmi’s picture

Munavijayalakshmi commented
Version: 9.5.x-dev » 10.0.x-dev
Assigned: Unassigned » Munavijayalakshmi
Status: Needs review » Needs work
Munavijayalakshmi’s picture

Munavijayalakshmi commented
Assigned: Munavijayalakshmi » Unassigned
Munavijayalakshmi’s picture

Munavijayalakshmi commented

hi @shashwat-tiwari,
How to rectified error in #4.
can u please explain steps.
i am new to Drupal contributions.

shashwat-tiwari’s picture

shashwat-tiwari commented

hi @Munavijayalakshmi,

I followed the below steps:

  1. Applied the patch given at #4.
  2. Executed the shell command sh ./core/scripts/dev/commit-code-check.sh at root directory and validated the issue given at #4.
  3. Made the changes in core/themes/claro/css/components/form--text.pcss.css file.
  4. Executed the command yarn run build:css at core directory.
  5. Once step 4 is successful, I executed the shell command mentioned in Step 2 again and verified the code build success.
  6. Created the patch.
  7. Removed the patch files after taking the backup and applied the patch given at #4.
  8. Created a interdiff file for differences between the patch given at #4 and #6.

For 10.0.x, I'm creating a new patch because the existing patch is not getting applied for it.

shashwat-tiwari’s picture

shashwat-tiwari commented
Status: Needs work » Needs review
StatusFileSize
new 3294006-10.0.x-12.patch17.03 KB

Created the patch for 10.0.x.

Status: Needs review » Needs work

The last submitted patch, 12: 3294006-10.0.x-12.patch, failed testing. View results

aditya4478’s picture

aditya4478 commented
Version: 10.0.x-dev » 9.5.x-dev
Status: Needs work » Needs review
StatusFileSize
new 3294006.9.5.x.patch6.81 KB

Block for IE included. But there is some other error. I will post new patch soon

imalabya’s picture

imalabya
Primary language English
Location Bangalore
commented
Status: Needs review » Needs work

@Aditya4478 When you work on a patch on top of a previous patch, it is a good practice to add an interdiff file which will help the maintaners to review the new changes.
https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa...

aditya4478’s picture

aditya4478 commented
Version: 9.5.x-dev » 10.0.x-dev
StatusFileSize
new 3294006-10.0.x.patch6.36 KB

ignore this patch

aditya4478’s picture

aditya4478 commented
Version: 10.0.x-dev » 9.5.x-dev
Status: Needs work » Needs review
StatusFileSize
new 3294006-9.5.x.patch5.31 KB

ignore this patch

aditya4478’s picture

aditya4478 commented
StatusFileSize
new 3294006-version9.5.x.patch2.49 KB

wrong patch uploaded at wrong place. ignore this.

aditya4478’s picture

aditya4478 commented
Version: 9.5.x-dev » 10.0.x-dev
StatusFileSize
new 3294006-version9.5.x.patch5.67 KB

This is a patch of 9.5.x.
Nesting is done.

aditya4478’s picture

aditya4478 commented
StatusFileSize
new 3294006-version10.0.x.patch10.21 KB
sasanikolic’s picture

sasanikolic commented
Status: Needs review » Needs work 
+++ b/core/themes/claro/css/components/form--text.css
@@ -151,19 +138,25 @@ _:-ms-fullscreen,
-.form-element--type-textarea.error + .ck-editor > .ck-editor__main {

The selector here is wrong. This should result in just form-element--type-textarea.error + .ck-editor > .ck-editor__main. 

  1. +++ b/core/themes/claro/css/components/form--text.css
@@ -66,23 +66,29 @@
+   * Reset value border and background of the file input on IE11 and Edge.

    This results in an empty selector with a comment inside? This is weird... 

  2. +++ b/core/themes/claro/css/components/form--text.css
@@ -66,23 +66,29 @@
+   * Better upload button alignment for Chrome.

    Same as above.

aditya4478’s picture

aditya4478 commented
Version: 10.0.x-dev » 9.5.x-dev
Status: Needs work » Needs review
StatusFileSize
new 3294006.9.5.x.patch6.02 KB

changes done as per #21.

sasanikolic’s picture

sasanikolic commented
Status: Needs review » Needs work 
+++ b/core/themes/claro/css/components/form--text.css
@@ -66,23 +66,28 @@
+.form-element--type-file {
+  /**
+   * Reset value border and background of the file input on IE11 and Edge.

+++ b/core/themes/claro/css/components/form--text.pcss.css
@@ -55,18 +55,21 @@
+.form-element--type-file {
...
+  &::-ms-value {

I don't like these comments being compiled to the parent selector when nesting. I would suggest we switch those to inline comments - in that case they compile correctly next to the selector.

So that would be in pcss.css:

.form-element--type-file {
  &::-ms-value { /* ... */
    ... 
  }
}

I would like to get some other inputs in regards to that. Perhaps from @mherchel?

sakthivel m’s picture

sakthivel m commented
Status: Needs work » Needs review
StatusFileSize
new 3294006.9.5.x-24.patch6.45 KB
new reroll_diff_3294006.txt2.66 KB

#23 comments Fixed, Please verify the patch #24

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.

medha kumari’s picture

medha kumari
Location Jaipur
commented
Issue tags: -Needs reroll
StatusFileSize
new 3294006.9.5.x-26.patch637 bytes
new reroll_diff_24-26.txt7.23 KB

Rerolled patch #24 in 10.1.x .

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

_utsavsharma commented
StatusFileSize
new 3294006-29.patch1.26 KB

Rerolled patch #24 in 10.1.x .
Please review.

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.

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.

aditya4478’s picture

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

gauravvvv commented
Status: Needs work » Needs review
StatusFileSize
new 3294006-33.patch1.92 KB
new interdiff-29_33.txt1.17 KB

Updated logical properties in #29, attached interdiff for same. please review

Harish1688’s picture

Harish1688 commented
StatusFileSize
new 3294006 after patch.png9.12 KB

Hi,
After applying the patch 3294006-33.patch, the following points were observed:

1. CSS Logical Properties and nesting are utilized appropriately. (mostly all classes have variables, so found few changes on css Properties - like margin and text-indent)
2. The UI and focus states remain unchanged, showing no difference in behavior compared to the previous version.

Screenshot attached

looks good to RTBC

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Needs review 
+++ b/core/themes/claro/css/components/form--text.pcss.css
@@ -50,7 +50,7 @@
+  text-indent: calc(0.75rem - 0.0625rem); /* Text-input fallback for non-supporting browsers like Safari */

Why is this not using the variable anymore?

gauravvvv’s picture

gauravvvv commented
StatusFileSize
new 3294006-37.patch1.17 KB
new interdiff-33_37.txt1.26 KB

Updated the variable, added the interdiff for same

smustgrave’s picture

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

Restoring to RTBC

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 37: 3294006-37.patch, failed testing. View results

gauravvvv’s picture

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

Restoring to RTBC

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 37: 3294006-37.patch, failed testing. View results

gauravvvv’s picture

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

  • lauriii committed 0d20b5b4 on 11.x 
    Issue #3294006 by Aditya4478, Gauravvvv, shashwat-tiwari, Sakthivel M,...
lauriii’s picture

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

I don't think we need a follow-up here because inputs are spread across multiple files, and we can't really utilize component level custom properties for that reason.

Committed 0d20b5b and pushed to 11.x. Thanks!

Status: Fixed » Closed (fixed)

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