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
#20 table-drag-after.png30.49 KBrupeshghar
#20 table-drag-before.png30.54 KBrupeshghar
#18 3332473-nr-bot.txt1.61 KBneeds-review-queue-bot
#6 after-table-file.png71.09 KBsmustgrave
#6 before-table-file.png75.54 KBsmustgrave

Issue fork drupal-3332473

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 commented
Version: 10.0.x-dev » 10.1.x-dev

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

Gauravvv opened merge request !3487

gauravvvv’s picture

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

smustgrave commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new before-table-file.png75.54 KB
new after-table-file.png71.09 KB

Nesting looks great.
Uploading before/after to show nothing broke.

Looks good!

nod_’s picture

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

Small question for RTL styles

gauravvvv’s picture

gauravvvv commented

In the current CSS, it is left in the RTL dir.

[dir="rtl"] .table-file-multiple-widget .tabledrag-changed {
  float: left;
}
gauravvvv’s picture

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

santosh_verma commented

working on it

smustgrave’s picture

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

Changes appear to be 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.

lauriii’s picture

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

Posted review on the MR

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review

Addressed feedback from comment #11. Please review

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Threads appear to be addressed and verified the file widget is still working. Not uploading another set of screenshots unless needed.

lauriii’s picture

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

I think this comment still needs to be addressed: https://git.drupalcode.org/project/drupal/-/merge_requests/3487#note_195852

gauravvvv’s picture

gauravvvv commented

I have addressed the feedback from #16, please review

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3332473-nr-bot.txt1.61 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.

rupeshghar’s picture

rupeshghar commented
Assigned: Unassigned » rupeshghar

i am looking into this issue

rupeshghar’s picture

rupeshghar commented
StatusFileSize
new table-drag-before.png30.54 KB
new table-drag-after.png30.49 KB
rupeshghar’s picture

rupeshghar commented
Assigned: rupeshghar » Unassigned
Status: Needs work » Needs review
smustgrave’s picture

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

All threads have been addressed. Left a comment about the one blocking question saying it's been updated.

Believe this is good.

nod_’s picture

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

Problem with the float value, postcss and RTL styles.

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

mithun s’s picture

mithun s
Primary language English
Location Bangalore
commented
Status: Needs work » Needs review
nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Needs work

Tried to change the branch target but it's too old. The float values needs to be left and right explicitly. Since postcss doesn't support inline-start/end we should just not use it at all.

Mithun S opened merge request !8408

mithun s’s picture

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

Raised the PR against 11.x and updated the PR with changes. Please review.

smustgrave changed the visibility of the branch 3332473-refactor-claros-table--file-multiple-widget to hidden.

smustgrave’s picture

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

Believe feedback from previous MR has been addressed

nod_ closed merge request !8408

  • nod_ committed bc4cfe0e on 10.3.x 
    Issue #3332473 by Mithun S, Gauravvvv, rupeshghar, smustgrave, lauriii:...

  • nod_ committed 6f52b5e5 on 10.4.x 
    Issue #3332473 by Mithun S, Gauravvvv, rupeshghar, smustgrave, lauriii:...

  • nod_ committed e52c916f on 11.0.x 
    Issue #3332473 by Mithun S, Gauravvvv, rupeshghar, smustgrave, lauriii:...

  • nod_ committed 901d6d8a on 11.x 
    Issue #3332473 by Mithun S, Gauravvvv, rupeshghar, smustgrave, lauriii:...
nod_’s picture

nod_
Primary language French
Location Lille
commented
Version: 11.x-dev » 10.3.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 901d6d8aff to 11.x and e52c916fa6 to 11.0.x and 6f52b5e510 to 10.4.x and bc4cfe0ea3 to 10.3.x. Thanks!

Status: Fixed » Closed (fixed)

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