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.

Issue fork drupal-3332426

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:

Comments

Stockfoot created an issue. See original summary.

bspeare’s picture

Version: 10.0.x-dev » 10.1.x-dev

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

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

royalpinto007’s picture

Status: Active » Needs work

This change ensures that the margin of the node__submitted class only affects the top margin, which can improve consistency and readability. By using margin-top instead of margin, we can also reduce the potential for unintended side effects or conflicting styles that may arise from using the shorthand margin property.

hot_sauce’s picture

@royalpinto007

Switching to margin-top negates the bottom margin being set, which is needed, and also sets an inherited margin for the side margins, which may be need to be set as 0.

We could refactor this to use:

margin-block: 1em;
margin-inline: 0;

But that is the same as what this file has currently of margin: 1em 0

I think the shorthand for this file is fine and there may not be any actual changes needed here.

gauravvvv’s picture

I think the shorthand for this file is fine and there may not be any actual changes needed here.

I agree with @hotsaucedesign here. This file doesn't need any modifications. It's already using shorthand property.

hot_sauce’s picture

Status: Needs work » Closed (works as designed)

Thank you @Gauravvv, will mark this one as Closed (works as designed)