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
#36 views-ui-dialog-RTL-after.png170.97 KBakshayadhav
#36 views-ui-dialog-RTL-before.png183.37 KBakshayadhav
#36 views-ui-dialog-after.png175.8 KBakshayadhav
#36 views-ui-dialog-before.png182.3 KBakshayadhav
#36 views-ui-RTL-after.png341.32 KBakshayadhav
#36 views-ui-RTL-before.png342.03 KBakshayadhav
#36 views-ui-after.png385.88 KBakshayadhav
#36 views-ui-before.png385.56 KBakshayadhav
#18 3332743-views-ui_RTL-after.png726.85 KBakshayadhav
#18 3332743-views-ui_RTL-before.png757.3 KBakshayadhav
#18 3332743-views-ui_after.png317.88 KBakshayadhav
#18 3332743-views-ui_before.png317.87 KBakshayadhav
#18 interdiff_15-18.txt1.79 KBakshayadhav
#18 3332743-18.patch33.43 KBakshayadhav
#15 3332743-15.patch33.34 KBstanzin
#11 interdiff-9_11.txt16.76 KBgauravvvv
#11 3332743-11.patch34.73 KBgauravvvv
#10 Screen Shot 2023-03-03 at 12.06.05 PM.png408.82 KBsmustgrave
#9 3332743-9.patch26.57 KBgauravvvv
#6 3332743-6.patch5.65 KBdjsagar
#6 interdiff_4-6.txt471 bytesdjsagar
#4 3332743-4.patch5.85 KBgauravvvv

Issue fork drupal-3332743

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.

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

Gauravvv opened merge request !3231

gauravvvv’s picture

gauravvvv commented
StatusFileSize
new 3332743-4.patch5.85 KB

Improved CSS nesting in the file. Attached patch for same.

gauravvvv’s picture

gauravvvv commented
Status: Active » Needs review
djsagar’s picture

djsagar commented
StatusFileSize
new interdiff_4-6.txt471 bytes
new 3332743-6.patch5.65 KB

Tried to fix CCF for #4.

Status: Needs review » Needs work

The last submitted patch, 6: 3332743-6.patch, failed testing. View results

bspeare’s picture

bspeare commented
Version: 10.0.x-dev » 10.1.x-dev
gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review
StatusFileSize
new 3332743-9.patch26.57 KB

I have refactored the code, please review

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative
StatusFileSize
new Screen Shot 2023-03-03 at 12.06.05 PM.png408.82 KB

This could use more nesting. Lost of instances (too many to full note each) but example
.views-config-group-region .views-group-box--value > .form-item::before {
.views-config-group-region .views-group-box--value > .form-item::after {

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review
StatusFileSize
new 3332743-11.patch34.73 KB
new interdiff-9_11.txt16.76 KB

Tried to improve nesting. attached interdiff for same

smustgrave’s picture

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

Nesting seems much better.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: 3332743-11.patch, failed testing. View results

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
Status: Needs work » Needs review
StatusFileSize
new 3332743-15.patch33.34 KB

improvised nesting :)

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Please provide an interdiff.

And since we have changes again will probably need another before/after screenshots.

akshayadhav’s picture

akshayadhav
he/him
Location Pune, India 🇮🇳
commented

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

akshayadhav’s picture

akshayadhav
he/him
Location Pune, India 🇮🇳
commented
Status: Needs work » Needs review
StatusFileSize
new 3332743-18.patch33.43 KB
new interdiff_15-18.txt1.79 KB
new 3332743-views-ui_before.png317.87 KB
new 3332743-views-ui_after.png317.88 KB
new 3332743-views-ui_RTL-before.png757.3 KB
new 3332743-views-ui_RTL-after.png726.85 KB

Made required changes. Removed @nest from the code as per guidelines mentioned here.
Please find attached images for before and after of both LTR and RTL language patterns.

AkshayAdhav changed the visibility of the branch 3332743-claro-views-ui-refactor to hidden.

akshayadhav’s picture

akshayadhav
he/him
Location Pune, India 🇮🇳
commented
Issue tags: +ClaroContributionDay2023

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

finnsky’s picture

finnsky commented

Converted #18 to MR

finnsky opened merge request !5821

finnsky changed the visibility of the branch 3332743-refactor-claros-views-ui to hidden.

finnsky’s picture

finnsky commented
Status: Needs review » Needs work

Gonna work on it a bit

finnsky’s picture

finnsky commented

Switching to another issue.
It still needs work at least in terms of source file formatting and variables usage.

akshayadhav’s picture

akshayadhav
he/him
Location Pune, India 🇮🇳
commented

Let me take a look into it again.

akshayadhav’s picture

akshayadhav
he/him
Location Pune, India 🇮🇳
commented
Status: Needs work » Needs review

Hi @finnsky
I have made the required changes for file formatting as well as tried to use variables wherever possible. Apart from properties where values are in decimals.

Also, I have kept the padding-bottom property as it is as per the comment on top of it.

.filterable-option .form-item.form-type-checkbox {
  padding-block: calc(var(--space-xs) / 2);
  /* This selector is aggressive because Claro's reset for .form-items is aggressive. */
  padding-bottom: calc(var(--space-xs) / 2);
  padding-inline-start: calc(var(--space-xs) / 2);
}
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs screenshots

With latest changes thing we need new screenshots

Just one set

gauravvvv’s picture

gauravvvv commented

Updated some logical properties and removed duplicated selectors. Still needs screenshots.

akshayadhav’s picture

akshayadhav
he/him
Location Pune, India 🇮🇳
commented

Hi @Gauravvvv
We should not include @nest. That's why I have removed it in my previous commit.
You can check this link for reference.

finnsky’s picture

finnsky commented

Nice catch @AkshayAdhav

Probably you can create issue to remove @nest usage?

akshayadhav’s picture

akshayadhav
he/him
Location Pune, India 🇮🇳
commented

@finnsky I think we should make @nest related changes while refactoring only.
But I guess most of us are missing this point, so we can create new issues if required.

finnsky’s picture

finnsky commented

Yep. I've checked. Sadly lot of that @nest were added in recent refactor tickets :)

akshayadhav’s picture

akshayadhav
he/him
Location Pune, India 🇮🇳
commented

Damn... Then we need to create child tickets for each refactor related issue tickets.

akshayadhav’s picture

akshayadhav
he/him
Location Pune, India 🇮🇳
commented
Status: Needs work » Needs review
StatusFileSize
new views-ui-before.png385.56 KB
new views-ui-after.png385.88 KB
new views-ui-RTL-before.png342.03 KB
new views-ui-RTL-after.png341.32 KB
new views-ui-dialog-before.png182.3 KB
new views-ui-dialog-after.png175.8 KB
new views-ui-dialog-RTL-before.png183.37 KB
new views-ui-dialog-RTL-after.png170.97 KB

@smustgrave attaching the screenshots with the latest changes.

gauravvvv’s picture

gauravvvv commented

Hi @Gauravvvv
We should not include @nest. That's why I have removed it in my previous commit.
You can check this link for reference.

We discussed this some time back, please see Drupal slack Conversation also, an issue.

smustgrave’s picture

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

Also as noted on a few tickets and in slack now. The @nest tickets should be consolidated into one.

Changes here seem fine thanks for the screenshots.

quietone’s picture

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

I'm triaging RTBC issues. I read the IS.

I then read the MR and found an @todo with a link to a closed issue. That is not a review as I don't do front end. I am setting this to needs work.

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review

Addressed feedback of #39. please review

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Now it points to a closed issue.

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review

Now it points to a closed issue.

The reference is correct; however, the earlier issue it pertains to is a duplicate of this closed issue. We can create a new issue to address that.

smustgrave’s picture

smustgrave commented

So we are saying this will be revisited in a closed ticket? Therefore it’ll never actually be revisited.

So since the referenced issue is closed we may need to revisit it now in this ticket.

gauravvvv’s picture

gauravvvv commented
Status: Needs review » Needs work

So we are saying this will be revisited in a closed ticket? Therefore it’ll never actually be revisited.

here I meant that we can create a new issue for this.

So since the referenced issue is closed we may need to revisit it now in this ticket.

Yes we can do it. Moving to NW to revisit this.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

quietone’s picture

quietone commented
Status: Needs work » Postponed

The Claro theme was approved for removal in #3576460: [policy, no patch] Deprecate and remove Claro.

This is Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.

The deprecation work is in #3576668: [meta] Tasks to deprecate Claro and the removal work in #3584638: [meta] Tasks to remove the Claro theme.