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

Created on 29 February 2024, 10 months ago
Updated 14 June 2024, 6 months ago

Problem/Motivation

If we use a default icon, then all new menu items who don't supply a custom icon in code, will have the same icon, which is very confusing.

Steps to reproduce

Proposed resolution

Instead of a default icon, use the first two letters of the menu item name.

Remaining tasks

User interface changes

API changes

Data model changes

Feature request
Status

Fixed

Version

11.0 🔥

Component
Navigation 

Last updated 2 days ago

No maintainer
Created by

🇺🇸United States KeyboardCowboy Denver, CO, USA

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @KeyboardCowboy
  • Assigned to bronzehedwick
  • 🇺🇸United States bronzehedwick New York
  • Issue was unassigned.
  • Status changed to Needs review 9 months ago
  • 🇺🇸United States bronzehedwick New York
  • Pipeline finished with Success
    9 months ago
    Total: 148s
    #120436
  • Status changed to Needs work 9 months ago
  • 🇨🇦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
  • 🇺🇸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!

  • Pipeline finished with Success
    9 months ago
    Total: 292s
    #122465
  • Status changed to RTBC 9 months ago
  • 🇨🇦Canada m4olivei Grimsby, ON

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

  • Status changed to Needs work 9 months ago
  • 🇪🇸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 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 🌱 [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 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 📌 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
  • 🇺🇸United States bronzehedwick New York
  • Pipeline finished with Failed
    9 months ago
    Total: 235s
    #123542
  • 🇷🇸Serbia finnsky

    this button can be without icon.

  • Pipeline finished with Failed
    9 months ago
    Total: 199s
    #123642
  • 🇺🇸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 a data-* 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 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.

  • Merge request !203SVG generation #3424744 → (Closed) created by finnsky
  • Status changed to Needs work 9 months ago
  • 🇷🇸Serbia finnsky

    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.

  • Pipeline finished with Success
    9 months ago
    Total: 204s
    #124563
  • 🇷🇺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 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 .

  • 🇪🇸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

  • 🇪🇸Spain ckrina Barcelona

    Moving this to major.

  • 🇪🇸Spain ckrina Barcelona
  • 🇪🇸Spain ckrina Barcelona
  • Merge request !7869Resolve #3424744 "On collapsed sidebars" → (Closed) created by m4olivei
  • 🇨🇦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.

  • Pipeline finished with Failed
    7 months ago
    Total: 171s
    #161661
  • 🇨🇦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.

  • Pipeline finished with Failed
    7 months ago
    Total: 151s
    #161667
  • 🇺🇸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 !7874Use abbreviation for default menu items → (Closed) created by bronzehedwick
  • Pipeline finished with Failed
    7 months ago
    Total: 172s
    #161773
  • Pipeline finished with Failed
    7 months ago
    Total: 190s
    #161776
  • Pipeline finished with Failed
    7 months ago
    Total: 169s
    #161795
  • Pipeline finished with Failed
    7 months ago
    #161829
  • Pipeline finished with Failed
    7 months ago
    Total: 1240s
    #161846
  • 🇨🇦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 .

  • Pipeline finished with Success
    7 months ago
    Total: 661s
    #161927
  • Status changed to Needs review 7 months ago
  • 🇺🇸United States bronzehedwick New York
  • Status changed to Needs work 7 months ago
  • 🇺🇸United States smustgrave

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

  • Pipeline finished with Failed
    7 months ago
    Total: 52s
    #162506
  • Pipeline finished with Failed
    7 months ago
    #163145
  • Pipeline finished with Failed
    7 months ago
    Total: 54s
    #163146
  • Pipeline finished with Failed
    7 months ago
    Total: 56s
    #163148
  • Pipeline finished with Success
    7 months ago
    Total: 56s
    #163165
  • Pipeline finished with Success
    7 months ago
    Total: 566s
    #163157
  • Pipeline finished with Success
    7 months ago
    Total: 52s
    #163189
  • Pipeline finished with Success
    7 months ago
    Total: 56s
    #163193
  • Pipeline finished with Success
    7 months ago
    Total: 337s
    #163224
  • Pipeline finished with Success
    7 months ago
    #163287
  • Pipeline finished with Success
    7 months ago
    Total: 54s
    #163300
  • Status changed to Needs review 7 months ago
  • 🇺🇸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!

  • Pipeline finished with Success
    7 months ago
    Total: 544s
    #163842
  • 🇺🇸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.

  • Pipeline finished with Success
    7 months ago
    Total: 566s
    #165779
  • Status changed to Needs work 7 months ago
  • 🇺🇸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
  • 🇺🇸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.

  • Pipeline finished with Failed
    7 months ago
    Total: 508s
    #166534
  • Pipeline finished with Success
    7 months ago
    Total: 601s
    #166543
  • 🇪🇸Spain ckrina Barcelona

    ckrina changed the visibility of the branch 3424744-on-collapsed-sidebars-off-11.x to hidden.

  • Pipeline finished with Success
    7 months ago
    Total: 221s
    #173243
  • Pipeline finished with Success
    7 months ago
    Total: 233s
    #173251
  • Pipeline finished with Success
    7 months ago
    #173273
  • Pipeline finished with Success
    7 months ago
    #173277
  • Pipeline finished with Success
    7 months ago
    Total: 82s
    #173344
  • Pipeline finished with Failed
    7 months ago
    Total: 145s
    #173342
  • Pipeline finished with Canceled
    7 months ago
    Total: 50s
    #173350
  • Pipeline finished with Success
    7 months ago
    #173351
  • Pipeline finished with Success
    7 months ago
    Total: 213s
    #173355
  • Pipeline finished with Success
    7 months ago
    Total: 52s
    #174263
  • Pipeline finished with Success
    7 months ago
    Total: 56s
    #174266
  • Pipeline finished with Success
    7 months ago
    Total: 55s
    #178956
  • Merge request !8186Icon or text - #3424744 → (Closed) created by finnsky
  • Status changed to Needs review 7 months ago
  • 🇷🇸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:

  • Pipeline finished with Success
    7 months ago
    Total: 659s
    #182411
  • Status changed to Needs work 7 months ago
  • 🇪🇸Spain plopesc Valladolid

    Thank you!

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

    I see 2 missing points though:

  • 🇷🇸Serbia finnsky

    Gonna review linked issue aswell

  • Status changed to Needs review 7 months ago
  • 🇷🇸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
  • 🇺🇸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
  • 🇷🇸Serbia finnsky

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

    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.

  • 🇺🇸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.

  • Pipeline finished with Success
    7 months ago
    Total: 32570s
    #185607
  • Status changed to RTBC 7 months ago
  • 🇺🇸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.

  • Pipeline finished with Success
    7 months ago
    Total: 552s
    #186128
  • Status changed to Needs work 6 months ago
  • 🇪🇸Spain ckrina Barcelona

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

  • Status changed to RTBC 6 months ago
  • 🇪🇸Spain ckrina Barcelona

    Applied the small changes myself.

    • ckrina committed e72a0d0c on 10.3.x
      Issue #3424744 by bronzehedwick, ckrina, m4olivei, finnsky, smustgrave,...
    • ckrina committed b4ee91dd on 10.4.x
      Issue #3424744 by bronzehedwick, ckrina, m4olivei, finnsky, smustgrave,...
    • ckrina committed 9f2863ce on 11.0.x
      Issue #3424744 by bronzehedwick, ckrina, m4olivei, finnsky, smustgrave,...
    • ckrina committed 549f289e on 11.x
      Issue #3424744 by bronzehedwick, ckrina, m4olivei, finnsky, smustgrave,...
  • Status changed to Fixed 6 months ago
  • 🇪🇸Spain ckrina Barcelona

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

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

  • Pipeline finished with Success
    4 months ago
    Total: 141s
    #241831
  • Pipeline finished with Success
    4 months ago
    #241836
  • Pipeline finished with Success
    3 months ago
    Total: 158s
    #268335
  • Pipeline finished with Success
    3 months ago
    Total: 180s
    #268400
  • Pipeline finished with Success
    3 months ago
    Total: 163s
    #268441
  • Pipeline finished with Success
    3 months ago
    #269088
  • Pipeline finished with Success
    3 months ago
    Total: 165s
    #271401
  • Pipeline finished with Canceled
    3 months ago
    Total: 82s
    #279579
  • Pipeline finished with Success
    3 months ago
    Total: 361s
    #279580
  • Pipeline finished with Failed
    3 months ago
    #280035
  • Pipeline finished with Failed
    3 months ago
    #280112
  • Pipeline finished with Success
    3 months ago
    #280199
  • Pipeline finished with Success
    3 months ago
    Total: 323s
    #281093
  • Pipeline finished with Success
    3 months ago
    Total: 313s
    #281602
Production build 0.71.5 2024