Skip to content
Snippets Groups Projects

Issue #3483209 by mogtofu33, finnsky, pdureau: Navigation leverage icon core API

Closed Issue #3483209 by mogtofu33, finnsky, pdureau: Navigation leverage icon core API
2 unresolved threads
Closed Jean Valverde requested to merge issue/drupal-3483209:3483209-navigation-use-icon-api into 11.x
2 unresolved threads

Closes #3483209

Merge request reports

Approval is optional
Code Quality is loading
Test summary results are being parsed

Closed by Théodore BiadalaThéodore Biadala 2 months ago (Mar 4, 2025 1:50pm UTC)

Merge details

  • The changes were not merged into 11.x.

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
9 9 {% macro menu_items(items, attributes, menu_level) %}
10 10 {% for item in items %}
11 11
12 {% set icon_id = null %}
13 {% if item.original_link.pluginId is defined %}
14 {% set icon_id = item.original_link.pluginId|split('.')|last %}
15 {% elseif item.class is defined %}
16 {% set icon_id = item.class|split('.')|last %}
17 {% endif %}
  • 9 9 {% macro menu_items(items, attributes, menu_level) %}
    10 10 {% for item in items %}
    11 11
    12 {% set icon_id = null %}
    13 {% if item.original_link.pluginId is defined %}
    14 {% set icon_id = item.original_link.pluginId|split('.')|last %}
    • Splitting the menu item plugin id and taking the last part seems risky, b/c you're not guaranteed a unique identifier, so you may end up with duplicates.

    • Using the last part of the menu plugin link id does appear to present an issue that should be addressed. For instance, currently, the People menu link is identified by entity.user.collection. This logic will come up with an icon_id of collection, and the /core/modules/navigation/assets/icons/collection.svg gets used, which is an icon of two persons. That works, b/c out of the box there is only one top-level menu link plugin id ending in collection. However, there are many menu link ids that end in collection, and you can imagine a scenario where another top-level menu link is placed in the Admin menu and would end up using the same icon. As a contrived example, consider this alter hook:

      /**
       * Implements hook_menu_links_discovered_alter().
       */
      function mymodule_menu_links_discovered_alter(&$links) {
        if (isset($links['entity.block_content_type.collection'])) {
          $links['entity.block_content_type.collection']['parent'] = 'system.admin';
        }
      }

      This puts the "Block types" menu link (/en/admin/structure/block-content) at the top level where it gets placed into the Navigation and gets the collection.svg icon, which is quite unexpected:

      awkward_collection_icon

      Edited by m4olivei
    • To be clear, you can also run into this awkward situation if you just re-order your Admin menu and pull a menu item with id *.collection to the top level. eg.

      Monosnap_screencast_2025-03-03_09-18-14

    • Ivan Berdinsky changed this line in version 12 of the diff

      changed this line in version 12 of the diff

    • Fixed with revert to clean class

    • Looks good! Thanks.

    • Please register or sign in to reply
  • Ivan Berdinsky added 45 commits

    added 45 commits

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • m4olivei added 16 commits

    added 16 commits

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • Ivan Berdinsky added 22 commits

    added 22 commits

    Compare with previous version

  • added 1 commit

    Compare with previous version

  • m4olivei added 1 commit

    added 1 commit

    Compare with previous version

  • Ivan Berdinsky added 18 commits

    added 18 commits

    Compare with previous version

  • Please register or sign in to reply
    Loading