Enable second navigation menu on pages (#300)#309
Enable second navigation menu on pages (#300)#309alexgibson merged 3 commits intomozilla:masterfrom stephaniehobson:nav
Conversation
stephaniehobson
added
Review: S
|
Needs tests but the other code is ready for a first pass. |
alexgibson
left a comment
alexgibson
left a comment
There was a problem hiding this comment.
Code-wise I think this looks fine, although I have some slight reservations about the usefulness of a navigation organism that only contains a menu molecule, and nothing else. The only real functionality it provides here is hiding the menu on small screens behind a hamburger icon. I guess this may well be justification enough, but the alternative would be just to use a second Menu molecule on the page? This is kinda what I originally had in mind for the Menu being a separate thing, although whether or not this turns out to be more complicated than it's worth is another question. Either way, it's up to you here as you have a specific usecase in mind.
| navButtons[i].addEventListener('click', Navigation.onClick, false); | ||
| } | ||
| } | ||
| Navigation.setAria(); |
There was a problem hiding this comment.
Could setAria() be inside the if (navItemsLists.length > 0) {} conditional?
| <button class="mzp-c-navigation-menu-button" type="button" aria-controls="map-c-navigation-items">Menu</button> | ||
| <div class="mzp-c-navigation-logo"><a href="https://www.mozilla.org/">Mozilla</a></div> | ||
| {{#if logo}}<div class="mzp-c-navigation-logo"><a href="https://www.mozilla.org/">Mozilla</a></div>{{/if}} | ||
| <div class="mzp-c-navigation-items" id="mzp-c-navigation-items"> |
There was a problem hiding this comment.
We should probably remove ids like id="mzp-c-navigation-items" from the component if we're now using more than one in a single page?
There was a problem hiding this comment.
✅ ID is now configurable (we need to keep an ID for the ARIA to work properly)
| * Event handler for navigation menu button `click` events. | ||
| */ | ||
| Navigation.onClick = function(e) { | ||
| var thisNavItemList = e.target.parentNode.querySelector('.mzp-c-navigation-items'); |
There was a problem hiding this comment.
If there can now be multiple navigation menus on one page, should we pass thisNavItemList as a parameter to onNavOpen and onNavClose, so those callbacks can work out which menu was interacted with?
|
I do want the collapse functionality for my use case. However, I also misunderstood the documentation on the Menu page thinking only the part that displays on hover is the menu. I will add a visual example to that page of the entire menu component since I found it hard to synthesize the code example without it. |
- make navigation button code only open its associated menu - update navigation template macro - add download button and mozilla logo as optional parameters - remove id from template as it was not being consumed - add second menu to navigation demo page
|
Updated. |
2 participants
Uh oh!
There was an error while loading. Please reload this page.