Problem/Motivation

1. Complex and monolithic javascript. The same functions are performed from several places independently.
2. Functional CSS contains many cascades and unreasonable increases in specificity, which makes it very difficult to maintain and extend.
3. aria attributes for menu links and buttons do not work when the URL is active.
4. We clone the submenu item in its folded state. Which is bad for both a11y and support.
5. We have a toolbar-link element which in fact does not exist without a parent menu and also has its logic spread out over several places.
6. The script for the menu and popover is the same, although they don’t have much in common

Proposed resolution

I divided the code into small and very simple components. CSS and JS which consists of one level. This is achieved by using customEvents. Similar to what we did in the wrapper script.

1. Tooltip. Which is added https://www.drupal.org/project/navigation/issues/3393067
2. Main button https://www.drupal.org/project/navigation/issues/3393216

3. Simple menu with simple styles.
The script contains only the functionality of opening a child menu and a trigger in case of an active link.

4. Popover with simple styles and script. Where most of the logic from the previous script lies.

5. toolbar-link has been removed because it is not needed. It doesn't exist outside of the menu.

Small improvements that have been added:

1. Added font size for toolbar-button, reduced specificity of display properties
2. used aria-expanded and aria-controls to display the visibility and state of menu arrows.
3. Used currentColor for menu item icons and hover elements. This made it possible to set the color of states from one place

This is a continuation of the work from here https://www.drupal.org/project/navigation/issues/3386927#comment-15280734
There you can see the motivation and logic in more detail.

The MR turned out to be quite large. But this is a minimally significant MR because everything is very strongly connected and confused in the current implementation.
Everything works as it always did, but the code has become much simpler and more predictable

Issue fork navigation-3401159

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

finnsky created an issue. See original summary.

finnsky’s picture

Status: Active » Needs review
ckrina’s picture

Status: Needs review » Needs work

it is ok in terms of BEM:

level - modifier name

2 - modifier value

TIL that you have to do that separation! :D https://en.bem.info/methodology/naming-convention/#two-dashes-style

I've just left a few comments about comments, so moving to NW just for that. Basically, the idea behind those comments is that any newbie (or just somebody without enough context) landing on that file in the future can understand what each part is doing. We don't have external documentation, so we rely on the fields themself to be the documentation itself. Olivero's navigation JS file is a great example: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/themes/oliver...

About the rest, everything makes a lot of sense to me: I personally don't see anything I'd like to change. But it would be great if somebody else could take a look to this, so I'll ping a few people in Slack to see if they can give a quick look to this.

Thanks for the proposal, looks great!

finnsky’s picture

Status: Needs work » Needs review

I added few comments and simplified js.
I think we may need dedicated ticket about js documentation according core standards.

ckrina’s picture

This looks good to go for me since it's a net improvement, thanks @finnsky! I'll give some time before merging to see if somebody else finds something worth being improved.

  • 2cecbbc2 committed on 1.x
    Refactor components - #3401159
    
ckrina’s picture

Status: Needs review » Fixed

OK, time to merge this. Thanks for the work @finnsky!

Status: Fixed » Closed (fixed)

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