Problem/Motivation

This is part of the CSS modernization initiative, and intended to be worked on by our Google Summer of Code student only. This is intended to be a straightforward first issue to easily onboard the student.

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.

CommentFileSizeAuthor
#27 3284881-10-0-x-version.patch2.92 KBaditya4478
#26 3284881.patch2.58 KBaditya4478
#25 Edited.patch2.75 KBaditya4478
#24 3284881.patch5.36 KBaditya4478
#23 new.patch2.68 KBaditya4478
#22 3284881.patch5.36 KBaditya4478
#21 notepad-check.patch2.68 KBaditya4478
#19 temp.patch5.36 KBaditya4478
#18 3284881-10.0.x-version.patch6.04 KBaditya4478
#16 3284881-latest.patch6.04 KBaditya4478
#14 3284881-latest.patch5.36 KBaditya4478
#11 interdiff_6-11.txt349 bytespooja saraah
#11 3284881-11.patch2.41 KBpooja saraah
#6 3284881.patch2.5 KBaditya4478
#4 3284881.patch2.67 KBaditya4478
Create_Article___Olivero.png235.99 KBmherchel

Comments

mherchel created an issue. See original summary.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented

This issue is for the Drupal Association's Google Summer of Code student. Please don't work on this unless you are him!

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Issue summary: View changes
Issue tags: +Needs followup
aditya4478’s picture

aditya4478 commented
Status: Active » Needs review
StatusFileSize
new 3284881.patch2.67 KB

Patch Attached !! :)

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs review » Needs work 
+++ b/core/themes/claro/css/components/accordion.pcss.css
@@ -16,26 +16,26 @@
+  & .accordion__item .claro-details__summary .summary {

You're redeclaring the .accordion__item selector here. It's already the parent selector, so this is not needed.

aditya4478’s picture

aditya4478 commented
StatusFileSize
new 3284881.patch2.5 KB

Changed redeclared selectore

aditya4478’s picture

aditya4478 commented
Status: Needs work » Needs review
asishsajeev’s picture

asishsajeev commented

@Aditya4478 Patch applied successfully.

sasanikolic’s picture

sasanikolic commented 
  1. +++ b/core/themes/claro/css/components/accordion.css
@@ -20,27 +20,23 @@
+  border-radius: 0

    Missing ";" at the end of the css definition. 

  2. +++ b/core/themes/claro/css/components/accordion.pcss.css
@@ -16,26 +16,22 @@
 .accordion__item {

    Shouldn't this also be under .accordion? Like so: &__title { ... }

I also checked the css file locally and some curly brackets are missing. Let's try compiling once more and running the linter.

Also, let's make sure to post interdiffs between patches so we can see the change from patch 1 to patch 2. That's always very helpful to have.

sasanikolic’s picture

sasanikolic commented
Status: Needs review » Needs work
pooja saraah’s picture

pooja saraah commented
StatusFileSize
new 3284881-11.patch2.41 KB
new interdiff_6-11.txt349 bytes

Addressed the comment #9
Attached interdiff patch

sasanikolic’s picture

sasanikolic commented

@pooja saraah thanks for your patch, but please be aware that this is a task for our GSoC student and we'd like him to contribute to this task.

Checking the patch above, this piece of code got lost and needs to be added. 

.accordion__item + .accordion__item {
  margin-top: -1px;
}

And please disregard my comment regarding the &__title { ... } change, that can't be done with PostCSS.

sasanikolic’s picture

sasanikolic commented

Also, we need a separate patch for Drupal 10, which will contain different code.

aditya4478’s picture

aditya4478 commented
Status: Needs work » Needs review
StatusFileSize
new 3284881-latest.patch5.36 KB

Patch for Drupal 9

sasanikolic’s picture

sasanikolic commented
Status: Needs review » Needs work

The changes look good to me now.
We just need to make sure the patch applies correctly and please also provide another patch for Drupal 10.

aditya4478’s picture

aditya4478 commented
Status: Needs work » Needs review
StatusFileSize
new 3284881-latest.patch6.04 KB

Attached the patch for Drupal 10

aditya4478’s picture

aditya4478 commented
Version: 9.5.x-dev » 10.0.x-dev

Attached patch for Drupal 10.0.x (Ignore the previous one)

aditya4478’s picture

aditya4478 commented
StatusFileSize
new 3284881-10.0.x-version.patch6.04 KB

Sorry for the multiple Comments. Please ignore comment #16,#17
Consider this Patch for 10.0.x.

aditya4478’s picture

aditya4478 commented
Version: 10.0.x-dev » 9.5.x-dev
StatusFileSize
new temp.patch5.36 KB
sasanikolic’s picture

sasanikolic commented
Status: Needs review » Needs work

Hi @Aditya4478. I think your patches are failing because of bad encoding. Can you try to do the following and see if it helps? This did the trick for me and the patch was applied locally.

- Opened patch file with an editor like VS Code
- Changed encoding to UTF-8
- Changed line endings from CRLF to LF
- Saved the new file
- git apply myPatch.patch worked

Also this might be relevant to you, since you're on a Windows machine:

Quick solution in Windows:
- Open notepad
- Copy your git diff content to blank document in Notepad
- Save as a new file
- git apply the file
- It's important to copy content, not just to open the diff file in notepad.
aditya4478’s picture

aditya4478 commented
StatusFileSize
new notepad-check.patch2.68 KB

Checking Notepad method if it is works for me..

aditya4478’s picture

aditya4478 commented
StatusFileSize
new 3284881.patch5.36 KB

Patch with VS Code changes

aditya4478’s picture

aditya4478 commented
StatusFileSize
new new.patch2.68 KB
aditya4478’s picture

aditya4478 commented
StatusFileSize
new 3284881.patch5.36 KB
aditya4478’s picture

aditya4478 commented
Status: Needs work » Needs review
StatusFileSize
new Edited.patch2.75 KB
aditya4478’s picture

aditya4478 commented
StatusFileSize
new 3284881.patch2.58 KB
aditya4478’s picture

aditya4478 commented
Version: 9.5.x-dev » 10.0.x-dev
StatusFileSize
new 3284881-10-0-x-version.patch2.92 KB
sasanikolic’s picture

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

These changes look good to me except for the formatting and the semicolon missing, but that looks like a compilation issue.

  • lauriii committed 3f6a0e9 on 10.1.x 
    Issue #3284881 by Aditya4478, mherchel, sasanikolic: Refactor Claro's...

  • lauriii committed acf7dbe on 10.0.x 
    Issue #3284881 by Aditya4478, mherchel, sasanikolic: Refactor Claro's...

  • lauriii committed 5231931 on 9.5.x 
    Issue #3284881 by Aditya4478, mherchel, sasanikolic: Refactor Claro's...
lauriii’s picture

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

Committed #27 3f6a0e9 and pushed to 10.1.x and cherry-picked to 10.0.x. Also committed #26 to 9.5.x. Thanks!

Leaving needs work for opening the follow-up.

mherchel’s picture

mherchel
he/him
Primary language English
Location Gainesville, FL, US
commented
Status: Needs work » Fixed
Issue tags: -Needs followup

Followup opened at #3312586: Refactor Claro's accordion stylesheet to take advantage of component-level custom properties.

Status: Fixed » Closed (fixed)

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