If no icon for a top-level item is provided, use the first two letters

Looking nice! I noticed one thing between collapsed and not expanded. It looks like there is a differnce in line-height or something that is causing a slight shift in the items that are defaulting to two letters. Here are a couple of examples:

Thanks @m4olivei! I can confirm the issue. Investigating now.

I found the issue with layout shift on menu collapse, and pushed a commit to fix. Ready for another review. Thanks!

This is looking great to me! Thanks for the fix @bronzehedwick.

Discussed in Slack, so moving to NW again and assigning it to @bronzehedwick since he said he'll work on this.

ckrina
@Chris DeLuca @m4olivei wondering how would you envision, when this is in core, how contrib modules could add their icons with this change moving its definition to templates and away from CSS

Chris DeLuca
Ahh, good point. I hadn’t considered that. I think there’d need to be some more development on it, but my first idea would be to look in a YAML file for a mapping of menu item ids to SVG paths on the file system?

ckrina
Yeah, I think moving the traditional strategy using CSS to define icons on a twig template complicates extensibility from what we had so far and it’d need some discussion & work.
We have this Plan/Meta issue where all this could be discussed #3381186: [PLAN] Icons for the sidebar.

Chris DeLuca
We can also move back to icons defined in CSS. There’s some other disadvantages, but if it makes extensibility easier, maybe it’s worth it

ckrina
But the changes you are proposing are increasing the scope of the what we’ll need to have, so I wonder if there is any way to address the Initials issue without getting into the weeds for this longer conversation?
I totally agree that the technical strategy is way better, it’s just that I’m trying to think about a way to get to the Beta line without increasing the scope

Chris DeLuca
That’s fine, I can move the solution back to icons in CSS
I’ll work on that now, unless anyone objects

I've also created #3432173: Decide strategy to customize or provide 1st level menu items' icons as a follow-up. Thanks!

I've reverted back to the CSS icons, as per above. The solution for overriding icons is no longer elegant.

This method requires a little bit more work to extend the icons than the
original, default icon implementation, as the override will also need to
set ::before { content: '' } and hide the abbreviation, in addition to
setting --icon.

Open to other suggestions.

this button can be without icon. and without text. You can test with enbling top bar. there is same button for node edit actions.

[class*='toolbar-button--icon'] { targets the same elements now as .toolbar-button, so I was consolidating the styles. The code would have the exact same effect split across those two selectors.

We could use attr() in conjunction with a data-* to store the value, but it would still require the same ::before, so I don't see an advantage.

Could you take a look at https://gyazo.com/b860d449b39738c41e181ddb7c4601ae
As i said this button used in other places aswell. I don't really want to have it always iconic.
Imo should be special class or attribute attached into this button when no icon in sidebar menu only. like `--icon-failback` or something.

That would be great, but I'm not seeing how that's possible. We can't detect if an icon is intended from Twig, just print a class usable for icon styling from CSS. This can be solved in the future if we decide to go back to an inline SVG approach, but not with CSS icons.

I gonna take a look at this in next couple of days. Thank you.

Could you take a look at https://gyazo.com/b860d449b39738c41e181ddb7c4601ae
As i said this button used in other places aswell. I don't really want to have it always iconic.
Imo should be special class or attribute attached into this button when no icon in sidebar menu only. like `--icon-failback` or something.

Gotcha, I see the problem. I'll see if I can narrow the selector, but I don't think it's possible to move away from the ::before approach or that attr() will make an impact on this problem. I also don't see a way for an icon fallback to work without detecting which items have icons from Twig, which would need the larger refactor.

I've added a bit crazy solution with inline svg generation.
https://gyazo.com/98af20f551a6184be3df90b62bb33170

it has some problems like we cannot use `inter` font. only safe webfonts. but works good in terms of global button

Gonna think a bit more about it.

I this this task is related to this https://www.drupal.org/project/navigation/issues/3432173 where we need to define one global strategy how we can deliver own / custom icons.

1. background (mask)
2. iconic font
3. embed svg <- this is a winner by the way

And i think the initial approach with ::before pseudoselector directly on button itself (.toolbar-button) was a bit wrong. Since such kind of things (which should work with own icons, external icons and even 2 letters if no icon defined) requires own wrapper, like
<span class="toolbar-button__start">... icon or 2 letters ...</span>

What do you think? I think we can provide this extra wrapper directly in this task, it requires some rework of CSS but i don't think it will be hard & long

it has some problems like we cannot use `inter` font. only safe webfonts. but works good in terms of global button

Sorry, but UI design wise this needs to use the Inter font.

Also, be aware that the dropdown from the Top Bar and the 1st menu items on the navigation toolbar won't look the same and won't behave the same, so I wouldn't try to force them to use the same button component. Things like the states will need to be diffrerent also.

Can we get rid of ::before at all in toolbar buttons?
And to have just

<span class="icon">... optional 2 first letters...</span>

With the own tag and specific classname it would be much easier to deliver 2 letters. Also 100% it will be easier to integrate 3rd party icons.
Please share your opinion

Moving this to major.

I moved @bronzehedwick's MR over to Drupal core. There were quite a few conflicts, so I'm really not sure if everything is OK there. Take a look see.

To summarize the back channel in Slack. Now that we've got a clean branch agasint core to work with, we're seeing a failed test. That failed test is failing on account of changes to the Twig template that are including the abbreviation for the Top Bar button. The twig template changed gets used by the top bar (you need to drush en -y navigation_top_bar to see it on a node view/edit page). Probably don't want this in that case.

Should clean that up. Then we might still need to adjust the test depending.

@skaught correctly pointed out that we'll probably also want to move that failed test (NavigationTopBarTest) from where it is. That's the subject of #3444673: Navigation Test names are disjointed..

Reading the issue summary seems like something that could have test coverage added for.

@smustgrave I added an abbreviation test. It's my first time writing a Drupal test, so open to any feedback. Thanks!

@SKAUGHT aria-hidden="true" is applied to the .toolbar-button__abbreviation element. That should handle the use case you're mentioning.

Got around to reviewing the test and made a suggestion for a more open test name for future tests.

Also can we add before/after screenshots to the issue summary to show the issue, can reuse any of the existing files but should be in the UI section of the summary for quick reviews

On my side i still believe we should postpone it until icons logic will not decided:

https://www.drupal.org/project/drupal/issues/3432173

If this could happen independently it would be great since the existing icon for missing icons looks like "broken" or that an icon is missing. We can always refactor and integrate the logic in the SVG process that ends up being used.

Also, it would be great if this MR didn't change any SVG file that doesn't really need to change to make it easier to isolate the changes.

Also, it would be great if this MR didn't change any SVG file that doesn't really need to change to make it easier to isolate the changes.

Sorry about that @ckrina, those changes snuck in due to IDE automatically inserted newlines at the end of the file. I've pushed a commit to revert that, so those changes should no longer be in the diff.

I tried to solve this problem differently.

At the moment, the button--has-icon class does not mean that the variable is actually set, all we have is the presence of the --icon variable

I tried to combine
https://css-tricks.com/logical-operations-with-css-variables/
and a double gradient depending on the presence of the variable and everything seems to work well.

Please review!

For test add any 2nd level menu link to Administration menu:

Thank you!

Tested locally and works as expected with the current icons approach.

I see 2 missing points though:

• We need tests for this
• We need to confirm that this approach is not creating conflicts with #3447837: Special Menu items are rendered as empty links in navigation, which is also affecting to toolbar-button.html.twig. However, we cannot work on those possible conflicts until one of them is merged

Gonna review linked issue aswell

1. Imo only Nightwatch tests can cover it. So we need to write test for this after Nightwatch setup will be merged.
2.

However, we cannot work on those possible conflicts until one of them is merged

We can work on both in same time.

@finnsky there an issue tracking the nightwatch setup? Should this be postponed on that?

@smustgrave,
this is Nightwatch issue: https://www.drupal.org/project/drupal/issues/3393400

But this issue shouldn't be blocked by Nightwatch setup.
We had that type of attributes before.

I'm not even sure this feature can be tested by Nightwatch.
Tricky CSS there, until we don't have Library of icons in module.

So not really sure what can happen here this. Don't think it can move forward without test coverage.

Problem that:
1. We cannot add test coverage here. Only cover attribute presence. But it is 1% of feature. Probably it is possible to do with Nightwatch. Probably...
2. We already have that type of attributes without any test coverage. Tests shouldn't block it.
3. This is temporary solution until we not have icons library.

Added basic test to ensure that HTML attributes are being placed as expected.

Agree that this might not be enough to test the CSS behavior, but it checks the HTML structure and would avoid unwanted markup regressions.

Thanks sorry I should of been clear about the tests that was looking for that vs testing css changes.

1) Drupal\Tests\navigation\Kernel\NavigationMenuMarkupTest::testToolbarButtonAttributes
//li[contains(@class,'toolbar-block__list-item')]/a[@data-icon-text='ti']
Failed asserting that 0 matches expected 1.
/builds/issue/drupal-3424744/core/modules/navigation/tests/src/Kernel/NavigationMenuMarkupTest.php:159
FAILURES!
Tests: 1, Assertions: 6, Failures: 1.

Shows the coverage.

Applied a super nitpicky change directly.

Believe this is good to go.

2 small design change requests and this is good to go.

Applied the small changes myself.

Committed 549f289 to 11.x, 9f2863c to 11.0.x, b4ee91d to 10.4.x and e72a0d0 to 10.3.x. Thanks!