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/claro/css/components/action-link.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

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
#16 interdiff-14_16.txt48.63 KBgauravvvv
#16 3303541-16.patch63.19 KBgauravvvv
#14 interdiff-12_14.txt6.66 KBgauravvvv
#14 3303541-14.patch27.25 KBgauravvvv
#12 interdiff-8_12.txt6.18 KBgauravvvv
#12 3303541-12.patch24.48 KBgauravvvv
#8 3303541-8.patch26.28 KBpradhumanjain2311
#3 3303541-version10.0.x.patch78.16 KBaditya4478
#2 3303541-version9.5.x.patch76.75 KBaditya4478

Issue fork drupal-3303541

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

sasanikolic created an issue. See original summary.

aditya4478’s picture

aditya4478 commented
StatusFileSize
new 3303541-version9.5.x.patch76.75 KB

Needs Review

aditya4478’s picture

aditya4478 commented
Version: 9.5.x-dev » 10.0.x-dev
Status: Active » Needs review
StatusFileSize
new 3303541-version10.0.x.patch78.16 KB
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
smustgrave’s picture

smustgrave commented

Should these be updated for 10.1.x?

smustgrave’s picture

smustgrave commented
Version: 10.0.x-dev » 10.1.x-dev
Status: Needs review » Needs work

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.

Unfortunately the 10.0 window has based and tried running 10.1 tests for #3 but appears to have some errors.

Also there is a follow up needed

pradhumanjain2311’s picture

pradhumanjain2311 commented
StatusFileSize
new 3303541-8.patch26.28 KB

Fix patch #3 for 10.1.x.

pradhumanjain2311’s picture

pradhumanjain2311 commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Still needs the follow up. Please read the tags before rerolling.

ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Issue summary: View changes

Updated issue summary with CSS changes expected.

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review
StatusFileSize
new 3303541-12.patch24.48 KB
new interdiff-8_12.txt6.18 KB

Updated the selectors and short-hand properties in #8. Attached interdiff for same

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative 
.action-link {

  /* Action link states */
  &:hover {
    text-decoration: none;
    color: var(--color-absolutezero-hover);
    background-color: var(--color-bgblue-hover);
  }

  &:focus {
    position: relative;
    z-index: 1;
    text-decoration: none;
  }

  &:active {
    color: var(--color-absolutezero-active);
    background-color: var(--color-bgblue-active);
  }
}

/* Defaults for icons */
.action-link::before {
  position: relative;
  inset-block-start: 0.125rem;
  /* Set the proper vertical alignment */
  display: inline-block;
  width: 1em;
  height: 1em;
  margin-inline: calc(var(--space-s) - var(--space-m)) 0.5em;
  background-repeat: no-repeat;
  background-position: center;
  background-size: contain;
}

can be merged with line 33 I believe.

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review
StatusFileSize
new 3303541-14.patch27.25 KB
new interdiff-12_14.txt6.66 KB

I have improved the nesting. attached patch with interdiff. please review

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

More nesting can be done.

Code is too long but all the action-link--icon-* can be combined with their parents.

This many changes will need new before/after screenshots.

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review
StatusFileSize
new 3303541-16.patch63.19 KB
new interdiff-14_16.txt48.63 KB

Improved nesting.

smustgrave’s picture

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

Additional nesting looks much better

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 16: 3303541-16.patch, failed testing. View results

gauravvvv’s picture

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

Unrelated failure, restoring status.

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 16: 3303541-16.patch, failed testing. View results

quietone’s picture

quietone commented

@najni, This issue is using a patch workflow and was RTBC, there is no need for an MR. Therefor, credit has been removed per How is credit granted for Drupal core issues.

gauravvvv’s picture

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

Unrelated failure, Restoring status.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 16: 3303541-16.patch, failed testing. View results

nod_’s picture

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

  • nod_ committed bf01a581 on 10.1.x 
    Issue #3303541 by Gauravvvv, Aditya4478, pradhumanjainOSL, smustgrave:...
nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs followup

Works in RTL as well. Would have been nice to have screenshots, did my own testing anyway everything looks good.

Couldn't find where the followup was needed, or for what purpose. Removing the tag.

Committed bf01a58 and pushed to 10.1.x. Thanks!

ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented

The "Needs followup" label was added when the issue was created with this comment:

We need a followup issue to refactor this component in Drupal 10.0.x to make use of component-level CSS custom properties and remove IE specific style definitions.

Since this is exactly that, no followup needed. Thanks @nod_!

Status: Fixed » Closed (fixed)

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