- Issue created by @KeyboardCowboy
- Assigned to bronzehedwick
- Merge request !199Use first two letters of menu item as default icon ā (Closed) created by bronzehedwick
- Issue was unassigned.
- Status changed to Needs review
2 months ago 8:29pm 15 March 2024 - Status changed to Needs work
2 months ago 2:16am 16 March 2024 - šØš¦Canada m4olivei Grimsby, ON
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:
- šŗšøUnited States bronzehedwick
Thanks @m4olivei! I can confirm the issue. Investigating now.
- Status changed to Needs review
2 months ago 2:05pm 18 March 2024 - šŗšøUnited States bronzehedwick
I found the issue with layout shift on menu collapse, and pushed a commit to fix. Ready for another review. Thanks!
- Status changed to RTBC
2 months ago 2:47pm 18 March 2024 - šØš¦Canada m4olivei Grimsby, ON
This is looking great to me! Thanks for the fix @bronzehedwick.
- Status changed to Needs work
about 2 months ago 5:22pm 19 March 2024 - šŖšøSpain ckrina Barcelona
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 CSSChris 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 š± [PLAN] Icons for the sidebar Active .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 itckrina
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 scopeChris DeLuca
Thatās fine, I can move the solution back to icons in CSS
Iāll work on that now, unless anyone objectsI've also created š Decide strategy to customize or provide 1st level menu items' icons Active as a follow-up. Thanks!
- šŗšøUnited States bronzehedwick
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.
- Status changed to Needs review
about 2 months ago 5:39pm 19 March 2024 - šŗšøUnited States bronzehedwick
[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 adata-*
to store the value, but it would still require the same::before
, so I don't see an advantage. - š·šøSerbia finnsky
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 i sidebar only. - šŗšøUnited States bronzehedwick
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.
- š·šøSerbia finnsky
I gonna take a look at this in next couple of days. Thank you.
- šŗšøUnited States bronzehedwick
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 thatattr()
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. - Status changed to Needs work
about 2 months ago 8:25pm 20 March 2024 - š·šøSerbia finnsky
I've added a bit crazy solution with inline svg generation.
https://gyazo.com/98af20f551a6184be3df90b62bb33170it 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.
- š·šŗRussia kostyashupenko Omsk
I this this task is related to this https://www.drupal.org/project/navigation/issues/3432173 š Decide strategy to customize or provide 1st level menu items' icons Active 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 wayAnd 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 .
- šŖšøSpain ckrina Barcelona
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.
- š·šŗRussia kostyashupenko Omsk
Can we get rid of ::before at all in toolbar buttons?
And to have just
... optional 2 first letters...
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 - šØš¦Canada m4olivei Grimsby, ON
m4olivei ā changed the visibility of the branch 3424744-svg-generate to hidden.
- šØš¦Canada m4olivei Grimsby, ON
m4olivei ā changed the visibility of the branch 3424744-on-collapsed-sidebars to hidden.
- šØš¦Canada m4olivei Grimsby, ON
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.
- šŗšøUnited States bronzehedwick
bronzehedwick ā changed the visibility of the branch 3424744-on-collapsed-sidebars to hidden.
- šŗšøUnited States bronzehedwick
bronzehedwick ā changed the visibility of the branch 3424744-on-collapsed-sidebars to hidden.
- Merge request !7873Resolve #3424744 "On collapsed sidebars off 11.x" ā (Closed) created by bronzehedwick
- Merge request !7875Issue #3424744: Use abbreviation for default menu items ā (Open) created by bronzehedwick
- šØš¦Canada m4olivei Grimsby, ON
m4olivei ā changed the visibility of the branch 3424744-on-collapsed-sidebars-off-11.x to hidden.
- šØš¦Canada m4olivei Grimsby, ON
m4olivei ā changed the visibility of the branch drupal-3424744-3424744-on-collapsed-sidebars-off-11.x to hidden.
- šØš¦Canada m4olivei Grimsby, ON
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 š Navigation Test names are disjointed. Active .
- Status changed to Needs review
17 days ago 8:34pm 1 May 2024 - Status changed to Needs work
17 days ago 9:10pm 1 May 2024 - šŗšøUnited States smustgrave
Reading the issue summary seems like something that could have test coverage added for.
- Status changed to Needs review
15 days ago 7:51pm 3 May 2024 - šŗšøUnited States bronzehedwick
@smustgrave I added an abbreviation test. It's my first time writing a Drupal test, so open to any feedback. Thanks!
- šŗšøUnited States bronzehedwick
@SKAUGHT
aria-hidden="true"
is applied to the.toolbar-button__abbreviation
element. That should handle the use case you're mentioning. - Status changed to Needs work
11 days ago 2:14pm 7 May 2024 - šŗšøUnited States smustgrave
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
- š·šøSerbia finnsky
On my side i still believe we should postpone it until icons logic will not decided:
https://www.drupal.org/project/drupal/issues/3432173 š Decide strategy to customize or provide 1st level menu items' icons Active
- šŖšøSpain ckrina Barcelona
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.
- Assigned to bronzehedwick
- šŗšøUnited States bronzehedwick
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.
- šŖšøSpain ckrina Barcelona
ckrina ā changed the visibility of the branch 3424744-on-collapsed-sidebars-off-11.x to hidden.