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.

@todo: Add clear testing instructions to test this manually on the UI.

Proposed resolution

  • Use CSS Logical Properties where appropriate.
  • Use CSS nesting where appropriate.
  • Use existing variables (variables.pcss.css) where appropriate. Follow the proposed Drupal CSS coding standards to name the variables.
    • Add a comment when there's a value where there is not a variable like font-size: 1.23rem; /* @todo One off value. */
    • When possible, set variables at the root of the component and then map them to global theme variables:
      .entity-meta {
          --entity-meta-title-font-size: var(--font-size-h5);

          ... more style
        }

        .entity-meta__title {
          font-size: var(--entity-meta-title-font-size);
        }

Out of scope

  • Changing CSS classes
  • Drupal 9 patches

User interface changes

None. There should be no visual differences.
Please post before/after screenshots and make sure they look the same.

CommentFileSizeAuthor
#75 3332446-test(3).png220.86 KBnayana_mvr
#75 3332446-test(2).png1.54 MBnayana_mvr
#75 3332446-test(1).png1.35 MBnayana_mvr
#57 after.png236.81 KBsourojeetpaul
#57 before.png236.81 KBsourojeetpaul
#53 3332446-nr-bot.txt1.35 KBneeds-review-queue-bot
#40 before.png497.51 KBshweta__sharma
#40 after.png508.83 KBshweta__sharma
#34 refactore.patch6.9 KBmeeni_dhobale
#34 After-refactoring.png103.71 KBmeeni_dhobale
#34 previous-refactoring.png103.71 KBmeeni_dhobale
#31 3332446-nr-bot.txt90 bytesneeds-review-queue-bot
#27 3332446-nr-bot.txt2.1 KBneeds-review-queue-bot
#24 Screenshot 2023-04-23 at 6.32.35 PM.png272.75 KBsantosh_verma
#24 Screenshot 2023-04-23 at 6.19.40 PM.png284.11 KBsantosh_verma
#20 interdiff_3332446.txt1.26 KB_pratik_
#17 3332446-refactor-claros-system-admin--modules-16.patch14.43 KBsantosh_verma
#7 Screenshot 2023-02-21 at 11.58.33 AM.png401.47 KBgauravvvv
#7 Screenshot 2023-02-21 at 11.44.36 AM.png411.25 KBgauravvvv

Issue fork drupal-3332446

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

Stockfoot created an issue. See original summary.

bspeare’s picture

bspeare CreditAttribution: bspeare commented
Version: 10.0.x-dev » 10.1.x-dev

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

Gauravvv opened merge request !3480

gauravvvv’s picture

gauravvvv CreditAttribution: gauravvvv at Axelerant for Drupal India Association commented
Status: Active » Needs review
smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative, +Needs screenshots

As the .css changes so much definitely think this needs before/after screenshots.

gauravvvv’s picture

gauravvvv CreditAttribution: gauravvvv at Axelerant for Drupal India Association commented
Status: Needs work » Needs review
StatusFileSize
new Screenshot 2023-02-21 at 11.44.36 AM.png411.25 KB
new Screenshot 2023-02-21 at 11.58.33 AM.png401.47 KB

I have added before/patch screenshot.

Before

After

smustgrave’s picture

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

Thanks!

nod_’s picture

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

few pixel offset in LTR for the checkboxes and the description, new patch is more consistent between LTR and RTL so not an issue.

Question about unit mixing in a single rule on the MR.

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

sourabhjain’s picture

sourabhjain
Primary language English
CreditAttribution: sourabhjain at Valuebound for Valuebound commented
Status: Needs work » Needs review

Resolved the issue mentioned in #9

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ at Eclipse Foundation commented
Status: Needs review » Needs work

should have been more detailed in the feedback :p

gauravvvv’s picture

gauravvvv CreditAttribution: gauravvvv at Axelerant for Drupal India Association commented

Left a comment on MR @nod_

gauravvvv’s picture

gauravvvv CreditAttribution: gauravvvv at Axelerant for Drupal India Association commented
Status: Needs work » Needs review
smustgrave’s picture

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

@Gauravvvv #13 you say you left a comment but don't see it?

santosh_verma’s picture

santosh_verma CreditAttribution: santosh_verma at Material for Drupal India Association commented

working on it to address comment #12

santosh_verma’s picture

santosh_verma CreditAttribution: santosh_verma at Material for Drupal India Association commented
Status: Needs work » Needs review
StatusFileSize
new 3332446-refactor-claros-system-admin--modules-16.patch14.43 KB

Comment #12 addressed in this current patch.
Rem replaced with PX as suggest in the comment #12

smustgrave’s picture

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

This ticket was started in an MR and should be continued there. Especially since no interdiff was provided

@Gauravvvv what was the comment you mentioned?

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

_pratik_’s picture

_pratik_ CreditAttribution: _pratik_ as a volunteer and at Specbee for Drupal India Association commented
StatusFileSize
new interdiff_3332446.txt1.26 KB

Attached interdiff between last two commits,
Thanks

santosh_verma’s picture

santosh_verma CreditAttribution: santosh_verma at Material for Drupal India Association commented

@smustgrave Pcss.css not generating (0 2px 4px rgba(0, 0, 0, 0.1) into rem after compilation. I tasted in my local the output was 0 2px 0.25rem rgba(0, 0, 0, 0.1);.
you can also refer the commit #b3156b80

In the commit #75ef2986 PX converted with rem in the Pcss.css file and in my suggestion we can go with that.

gauravvvv’s picture

gauravvvv CreditAttribution: gauravvvv at Axelerant for Drupal India Association commented
Status: Needs work » Needs review

@Gauravvvv what was the comment you mentioned?

let's use px everywhere. Postcss will do the conversion. This should be 0 2px 4px rgba(0, 0, 0, 0.1);

Earlier it was same --module-table-filter-box-shadow: 0 0.125rem 0.25rem rgba(0, 0, 0, 0.1); and postcss converts it into --module-table-filter-box-shadow: 0 2px 0.25rem rgba(0, 0, 0, 0.1);. That's why I have used rem to avoid this.

Reason behind this is, any value below 4px is not compiling to px from rem unit.

smustgrave’s picture

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

Sounds good to me.

santosh_verma’s picture

santosh_verma CreditAttribution: santosh_verma at Material for Drupal India Association commented
StatusFileSize
new Screenshot 2023-04-23 at 6.19.40 PM.png284.11 KB
new Screenshot 2023-04-23 at 6.32.35 PM.png272.75 KB

@smustgrave your last comment #23 is response to #22 ? but you changed the status need review to need work if any change needed please mention so that we can work on this :)

I tested the #22 MR and working fine

Before
before

After
after

gauravvvv’s picture

gauravvvv CreditAttribution: gauravvvv at Axelerant for Drupal India Association commented

@smustgrove, you have updated the status to needs work and commented.

Sounds good to me.

Is the status update intentional or by mistake?

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs work » Needs review
needs-review-queue-bot’s picture

needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commented
Status: Needs review » Needs work
StatusFileSize
new 3332446-nr-bot.txt2.1 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

gauravvvv’s picture

gauravvvv CreditAttribution: gauravvvv at Axelerant for Drupal India Association commented
Status: Needs work » Needs review
smustgrave’s picture

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

Looks good!

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.

needs-review-queue-bot’s picture

needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3332446-nr-bot.txt90 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

meeni_dhobale’s picture

meeni_dhobale CreditAttribution: meeni_dhobale as a volunteer and at QED42 commented

Working on this issue as a part of Claro Contribution Day.

meeni_dhobale’s picture

meeni_dhobale CreditAttribution: meeni_dhobale as a volunteer and at QED42 commented
Issue tags: +ClaroContributionDay2023
meeni_dhobale’s picture

meeni_dhobale CreditAttribution: meeni_dhobale as a volunteer and at QED42 commented
StatusFileSize
new previous-refactoring.png103.71 KB
new After-refactoring.png103.71 KB
new refactore.patch6.9 KB

I refactor the .pcss.css file. I was trying to rebase the current branch but facing some issues, so adding patch for now.
Here is the previous module list file.

Here are the screenshots of the changes.

meeni_dhobale’s picture

meeni_dhobale CreditAttribution: meeni_dhobale as a volunteer and at QED42 commented
Status: Needs work » Needs review
smustgrave’s picture

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

We shouldn't revert back to the old way of doing things.

Issue might be the current MR is pointed at 10.1.x we need one for 11.x

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

psebborn opened merge request !5830

gauravvvv’s picture

gauravvvv CreditAttribution: gauravvvv at Axelerant for Drupal India Association commented

I have updated leftover CSS logical properties. please review

shweta__sharma’s picture

shweta__sharma CreditAttribution: shweta__sharma at OpenSense Labs commented
StatusFileSize
new after.png508.83 KB
new before.png497.51 KB

Tested MR 5830 and it is applied cleanly now stylesheet using logical properties and nesting CSS.

Attached after and before screenshots

Can we move this to RTBC now?

shweta__sharma’s picture

shweta__sharma CreditAttribution: shweta__sharma at OpenSense Labs commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented

If you feel you've done a full code review, nothing has broken, and the core gates have been met then you can RTBC it https://www.drupal.org/about/core/policies/core-change-policies/core-gates

shweta__sharma’s picture

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

Thanks @smustgrave for the guidance.

Moving this to RTBC LGTM

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ at Très Bien Tech commented
Status: Reviewed & tested by the community » Needs work

Mithun S made their first commit to this issue’s fork.

mithun s’s picture

mithun s
Primary language English
Location Bangalore
CreditAttribution: mithun s for QED42 commented
Status: Needs work » Needs review

Addressed the review comments and updated the PR. Changing the status to Needs review.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ at Très Bien Tech commented
Status: Needs review » Needs work

few details remaining

mithun s’s picture

mithun s
Primary language English
Location Bangalore
CreditAttribution: mithun s for QED42 commented
Status: Needs work » Needs review

Addressed all the review comments and pushed a commit. Changing the status to Needs review.
Thank you!

smustgrave’s picture

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

appears feedback has been addressed

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ at Très Bien Tech commented
Status: Reviewed & tested by the community » Needs work

regresson on the "show all column" button when viewing on the mobile view.

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

SandeepMahlawat’s picture

SandeepMahlawat CreditAttribution: SandeepMahlawat at Innoraft commented
Status: Needs work » Needs review

i have implemented the solution as prescribed please review.

needs-review-queue-bot’s picture

needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commented
Status: Needs review » Needs work
StatusFileSize
new 3332446-nr-bot.txt1.35 KB

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

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

sourojeetpaul’s picture

sourojeetpaul CreditAttribution: sourojeetpaul as a volunteer and at Innoraft for Drupal India Association commented
Status: Needs work » Needs review
smustgrave’s picture

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

Seems to have a test failure, may need a rebase but should be tested.

sourojeetpaul’s picture

sourojeetpaul CreditAttribution: sourojeetpaul as a volunteer and at Innoraft for Drupal India Association commented
Assigned: Unassigned » sourojeetpaul
StatusFileSize
new before.png236.81 KB
new after.png236.81 KB

@smustgrave, As far as I've tested nothing seems to be breaking! Attaching before and screenshits for reference.
Also seems like PhpUnit test is failing over here, but not sure why PhpUnit tests are failing when the MR only make chnages on PCSS and CSS files.
Before
After

sourojeetpaul’s picture

sourojeetpaul CreditAttribution: sourojeetpaul as a volunteer and at Innoraft for Drupal India Association commented
Assigned: sourojeetpaul » Unassigned
psebborn’s picture

psebborn CreditAttribution: psebborn at Zoocha commented
Status: Needs work » Needs review

I've rebased from 11.x which seems to has resolved the pipeline. Ready for review

smustgrave’s picture

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

Left comments on MR.

mithun s’s picture

mithun s
Primary language English
Location Bangalore
CreditAttribution: mithun s for QED42 commented
Assigned: Unassigned » mithun s
mithun s’s picture

mithun s
Primary language English
Location Bangalore
CreditAttribution: mithun s for QED42 commented
Assigned: mithun s » Unassigned
mithun s’s picture

mithun s
Primary language English
Location Bangalore
CreditAttribution: mithun s for QED42 commented
Status: Needs work » Needs review
smustgrave’s picture

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

Small comment.

sanket.tale made their first commit to this issue’s fork.

sanket.tale’s picture

sanket.tale CreditAttribution: sanket.tale at QED42 commented
Status: Needs work » Needs review
smustgrave’s picture

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

Seemed to generate wrong see .css file

sanket.tale’s picture

sanket.tale CreditAttribution: sanket.tale at QED42 commented
Status: Needs work » Needs review
smustgrave’s picture

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

Feedback appears to be adderessed.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ at Très Bien Tech commented
Status: Reviewed & tested by the community » Needs work

There is a couple of px of difference in RTL (3px difference on the checkbox position), not a big deal but would be nice to find why.

Couple of comments left.

I've given the same feedback twice in a couple of places. Please make sure feedback is kept when moving branches or trying to restart work somewhere.

sanket.tale’s picture

sanket.tale CreditAttribution: sanket.tale at QED42 commented
Status: Needs work » Needs review

smustgrave changed the visibility of the branch 3332446-refactor-claros-system-admin--modules to hidden.

smustgrave’s picture

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

Small nitpicky comments on the comments.

psebborn’s picture

psebborn CreditAttribution: psebborn at Zoocha commented
Status: Needs work » Needs review

I've actioned the above points (added back the comment) - see MR for an explanation of why I've placed it inside the selector.

nayana_mvr’s picture

nayana_mvr CreditAttribution: nayana_mvr at Material commented
Status: Needs review » Needs work
StatusFileSize
new 3332446-test(1).png1.35 MB
new 3332446-test(2).png1.54 MB
new 3332446-test(3).png220.86 KB

PHPUnit Functional Javascript failures shown in the MR are not reproducible in local. Command used vendor/bin/phpunit --configuration phpunit.xml {filename}. Attaching screenshots as evidence. Please let me know if there is any other way to reproduce it in local. Moving this ticket back to Needs Work so that someone else can also verify it once.

stanzin changed the visibility of the branch 3332446-refactor-claros-system-admin--modules-11x to hidden.

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

stanzin opened merge request !11270