Contrib.social

Refactor Claro's menus-and-lists stylesheet

Open on Drupal.org →
Created on 10 January 2023, almost 2 years ago
Updated 21 April 2023, over 1 year ago

Problem/Motivation

This is a child of 🌱 [META] Update Claro CSS with new coding standards Active and part of 🌱 [PLAN] Drupal CSS Modernization Initiative Active .

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.

📌 Task
Status

Fixed

Version

10.1 ✨

Component
Claro  →

Last updated about 10 hours ago

Created by

🇺🇸United States morganlyndel

Live updates comments and jobs are added and updated live.
  • CSS

    It involves the content or handling of Cascading Style Sheets.

Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • First commit to issue fork.
  • @bspeare opened merge request.
  • Comment almost 2 years ago →
    bspeare
  • Comment almost 2 years ago →
    System Message
  • @bspeare opened merge request.
  • Comment almost 2 years ago →
    🇮🇳India gauravvvv Delhi, India

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

  • Status changed to Needs review almost 2 years ago
  • Comment almost 2 years ago →
    🇮🇳India gauravvvv Delhi, India

    I have improved nesting and added margin-inline-start & margin-inline-end, so we don't need 

    [dir="rtl"] .item-list ul {
  margin: 0.25em 1.5em 0.25em 0;
}

    anymore.

    Please review

  • Status changed to Needs work almost 2 years ago
  • Comment almost 2 years ago →
    🇺🇸United States smustgrave

    Seems more nesting could be done I think.

    Also does [dir="rtl"] need to be replaced?

  • Status changed to Needs review almost 2 years ago
  • Comment almost 2 years ago →
    🇮🇳India gauravvvv Delhi, India
  • Status changed to Needs work almost 2 years ago
  • Comment almost 2 years ago →
    🇺🇸United States smustgrave

    Issue summary mentions needing before/after screenshots to show nothing broke.

    Code wise everything looks fine.

  • Status changed to Needs review almost 2 years ago
  • Comment almost 2 years ago →
    🇮🇳India gauravvvv Delhi, India


    After patch:

    Added after patch screenshot, please review

  • Status changed to RTBC almost 2 years ago
  • Comment almost 2 years ago →
    🇺🇸United States smustgrave

    Will let committer decide but seems difficult to read. But not sure what could be done.

  • First commit to issue fork.
  • Status changed to Needs review over 1 year ago
  • Comment over 1 year ago →
    🇺🇸United States mherchel Gainesville, FL, US

    Did a bit of work on this.

    The first thing I noticed was image referenced in list-style-image: url(../../images/menu-collapsed.png); did not exist.

    This image should get called when there's a menu. Note that the menu just shows up as an unordered list, but the images (which were small triangles) didn't show. So, I removed them from being called.

    I also added spacing between the code blocks and converted the REM unit to PX (as it'll be auto converted by PostCSS.

  • First commit to issue fork.
  • Status changed to RTBC over 1 year ago
  • Comment over 1 year ago →
    🇺🇸United States smustgrave

    Tested on modules page and didn't see any issues.

  • Comment over 1 year ago →
    System Message
    • nod_ → committed bf266299 on 10.1.x 
      Issue #3332363 by Gauravvvv, mherchel, smustgrave: Refactor Claro's...
  • Comment over 1 year ago →
    🇫🇷France nod_ Lille

    Committed bf26629 and pushed to 10.1.x. Thanks!

    Thank you for your assistance on this issue VladimirAus

    Starting March 2023, simple rerolls, rebases, or merges will no longer receive issue credit. Only rerolls that address a merge conflict will be credited, and the merge conflict that was resolved must be documented in the text of an issue comment.

    To receive credit for contributing to this issue, assist with other outstanding tasks or unaddressed feedback.

    See the issue credit guidelines → for more information.

  • Status changed to Fixed over 1 year ago
  • Comment over 1 year ago →
    System Message

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

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024