Problem/Motivation

Follow-up from the core MR.
In core/modules/navigation/css/components/toolbar-button.pcss.css the code written in line 286 for .toolbar-button--collapsible .toolbar-button__label is reimplementing something the core class .visually-hidden .

Proposed resolution

Directly apply the class so no extra code is needed.

Remaining tasks

Issue fork drupal-3442931

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

ckrina created an issue. See original summary.

ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Project: Navigation » Drupal core
Version: 1.x-dev » 11.x-dev
Component: Code » navigation.module
ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Issue tags: +Portland2024

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

gauravvvv’s picture

gauravvvv commented
Status: Active » Needs review

Gauravvvv opened merge request !7954

finnsky’s picture

finnsky commented
Status: Needs review » Needs work

Thank you! I've added couple of comments.

finnsky’s picture

finnsky commented

In fact i would better to discuss it.

@ckrina

I think CSS solution of duplication .visually-hidden CSS properties looks cleaner than JS extra search of that labels.
Probably better to keep it as is.

gauravvvv’s picture

gauravvvv commented
Status: Needs work » Needs review

Addressed feedbacks in MR

smustgrave’s picture

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

May be a stretch but seems like something that a simple assertion could be added to a test.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Needs work » Closed (won't fix)

I was the one who asked for this, but I agree with #8
It's a shame we don't use https://www.npmjs.com/package/postcss-mixins because this would be a good case for it.

It's also a shame there's no display: visually-hidden in the platform, but I see there are folks proposing it so perhaps at some point we will be able to ditch this.