- 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
9 months ago 8:29pm 15 March 2024 - Status changed to Needs work
9 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 New York
Thanks @m4olivei! I can confirm the issue. Investigating now.
- Status changed to Needs review
9 months ago 2:05pm 18 March 2024 - 🇺🇸United States bronzehedwick New York
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
9 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
9 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 New York
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
9 months ago 5:39pm 19 March 2024 - 🇺🇸United States bronzehedwick New York
[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 New York
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 New York
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
9 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 New York
bronzehedwick → changed the visibility of the branch 3424744-on-collapsed-sidebars to hidden.
- 🇺🇸United States bronzehedwick New York
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
7 months ago 8:34pm 1 May 2024 - Status changed to Needs work
7 months 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
7 months ago 7:51pm 3 May 2024 - 🇺🇸United States bronzehedwick New York
@smustgrave I added an abbreviation test. It's my first time writing a Drupal test, so open to any feedback. Thanks!
- 🇺🇸United States bronzehedwick New York
@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
7 months 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 New York
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.
- Status changed to Needs review
7 months ago 6:43am 26 May 2024 - 🇷🇸Serbia finnsky
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!
- 🇷🇸Serbia finnsky
For test add any 2nd level menu link to Administration menu:
- Status changed to Needs work
7 months ago 5:43am 27 May 2024 - 🇪🇸Spain plopesc Valladolid
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
🐛
Special Menu items are rendered as empty links in navigation
RTBC
, which is also affecting to
toolbar-button.html.twig
. However, we cannot work on those possible conflicts until one of them is merged
- Status changed to Needs review
7 months ago 5:50am 27 May 2024 - 🇷🇸Serbia finnsky
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.
- Issue was unassigned.
- Status changed to Needs work
7 months ago 3:16pm 29 May 2024 - 🇺🇸United States smustgrave
@finnsky there an issue tracking the nightwatch setup? Should this be postponed on that?
- Status changed to Needs review
7 months ago 4:48pm 29 May 2024 - 🇷🇸Serbia finnsky
@smustgrave,
this is Nightwatch issue: https://www.drupal.org/project/drupal/issues/3393400 ✨ Evaluate adding Nightwatch tests ActiveBut 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. - 🇺🇸United States smustgrave
So not really sure what can happen here this. Don't think it can move forward without test coverage.
- 🇷🇸Serbia finnsky
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. - 🇪🇸Spain plopesc Valladolid
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.
- 🇺🇸United States smustgrave
smustgrave → changed the visibility of the branch drupal-3424744-drupal-3424744-3424744-on-collapsed-sidebars-off-11.x to hidden.
- Status changed to RTBC
7 months ago 1:53pm 30 May 2024 - 🇺🇸United States smustgrave
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.
- Status changed to Needs work
6 months ago 5:43pm 31 May 2024 - 🇪🇸Spain ckrina Barcelona
2 small design change requests and this is good to go.
- Status changed to RTBC
6 months ago 8:20pm 31 May 2024 - Status changed to Fixed
6 months ago 8:34pm 31 May 2024 Automatically closed - issue fixed for 2 weeks with no activity.