Some tasks to prepare for core MR, will probably complete this over time.

Must have

Done

  • In the JS all the element selectors should rely on data attributes and not class names, things such as .toolbar-menu__item.toolbar-menu__item--level-2 etc.
  • There are some syntax issues, ifs that are not wrapped with {}, multiple declarations of variables on one line. essentially the eslint core config should be run on the code and fixed.
    I don't have a good solution for that yet, the code could use a refactor to improve readability possibly grouping the positioning into one file/area of the code that sort of things.

Should have

Comments

nod_ created an issue. See original summary.

finnsky’s picture

finnsky commented

Refactor suggestion here
https://www.drupal.org/project/navigation/issues/3401159

ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented

We just discussed this with @finnsky and it looks like everything on the summary has been addressed so far:

In the JS all the element selectors should rely on data attributes and not class names, things such as .toolbar-menu__item.toolbar-menu__item--level-2 etc.

This has been done.

There are some syntax issues, ifs that are not wrapped with {}, multiple declarations of variables on one line. essentially the eslint core config should be run on the code and fixed.

I don't have a good solution for that yet, the code could use a refactor to improve readability possibly grouping the positioning into one file/area of the code that sort of things.

After componentizing the CSS and JS and breaking the JS in several files ("no monolithic js anymore" per @finnsky words) we think this is addressed too. The work happened in:

Maybe this would need another round to review what needs to happen for core, or otherwise we could close this one.

ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Status: Active » Needs review
ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Issue summary: View changes

Adding some JS related issues in the issue summary, on the Should have for now.

ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Title: Todo for core commit (JS code) » [PP1] Todo for core commit (JS code)
Issue summary: View changes
Status: Needs review » Postponed (maintainer needs more info)

Postponing this until we get #3412125: Convert jQuery to vanilla Javascript, so the code will be on a more finished state.

Also updating the issue summary.

nod_’s picture

nod_
Primary language French
Location Lille
commented

Been using the module for a few days, there is a perf issue. Font flashes on every page it's pretty annoying. The initialisation should be done outside of behaviors, that happens too late.

We did a lot of optimisation work on the core toolbar to avoid any flash or layout shift, we should do the same here before getting that in

ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Issue summary: View changes
Status: Postponed (maintainer needs more info) » Needs review
Related issues: +#3396174: The toolbar should be usable without JS

@nod_ there is an issue for that™ :P #3423020: Flickering when loading the page. Adding it to the issue summary, thanks!.

ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Issue summary: View changes
ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Title: [PP1] Todo for core commit (JS code) » Todo for core commit (JS code)
Status: Needs review » Active

Changing title & status since it's active.

ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented
Status: Active » Fixed

Closing this now that #3438895: Add the new Navigation to core as an Experimental module has been opened and all the JS feedback is going to be addressed from there.

ckrina’s picture

ckrina
she/her
Primary language Catalan
Location Barcelona
commented

Status: Fixed » Closed (fixed)

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