Problem/Motivation

Right now all the icons are embedded SVGs into the HTML. But in core we don't have a way to tie SVG to menu items, so the only way we'll have to provide icons for now will be the existing solution in the toolbar: with CSS backgrounds.

Proposed resolution

Move all SVG to CSS backgrounds with an strategy close to what the existing Toolbar uses.

Issue fork navigation-3379057

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
Priority: Normal » Minor

Moving to minor since it won't be necessary for testing purposes.

claireristow’s picture

claireristow commented
Assigned: Unassigned » claireristow

Working on this now

claireristow opened merge request !54

claireristow’s picture

claireristow commented
Status: Active » Needs review

Hey @ckrina, do you mind doing a quick review of my work? I'm wondering if you're ok with the mask-image approach I took for hover color changes or if you'd prefer to have multiple color versions of the icons, or if there's another option entirely!

ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Status: Needs review » Needs work

@claireristow nice work!

About the mas itself, I personally think it’s a really good modern strategy to avoid what core is currently doing by storing the same icon in several colors. This is assuming all icons will be monochromatic, which I’d say it’s a safe assumption. It is also good because it’ll let us play with accent colors in the future letting us customize the admin UI colors. So unless anybody else has more insights I don’t oversee any problem with this,

The only thing I'd change here is that I would use a BEM pattern to name icons, and use each icon name as a modifier. From navigation-icon-media I’d convert it to navigation-link--media.

You can see an example in action-link.pcss.css.

ckrina credited mherchel.

ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented

Make sure if the SVGs are inlined into the CSS, you abstract them to a CSS variable (so the long SVG doesn't get output twice). Also keep in mind you'll pretty much always want to use them in pseudo-classes because the mask covers up everything else.

This is @mherchel feedback in Slack :)

claireristow’s picture

claireristow commented

Thanks for the feedback @ckrina and @mherchel, I'll work on those changes!

claireristow’s picture

claireristow commented
Assigned: claireristow » Unassigned
Status: Needs work » Fixed

Merged updates to BEM-style classes and setup CSS variables for icons!

Status: Fixed » Closed (fixed)

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