NGSTACK-1015 Improve navigation accessibility and UX#270
Open
jakubraus wants to merge 2 commits into
Open
Conversation
Contributor
Author
|
Forgot to mention, even though I wouldn't recommend to use the split buttons (link AND submenu trigger), they are supported. But the preferred way is always to use either only links, or only submenu triggers, as the split buttons provide horrible UX. |
emodric
reviewed
Jan 27, 2026
| <li{{ knp_menu.attributes(attributes) }}> | ||
| {%- if item.uri is not empty and (not matcher.isCurrent(item) or options.currentAsLink) %} | ||
| {{ block('linkElement') }} | ||
| {%- elseif submenuId is defined %} |
Member
There was a problem hiding this comment.
Instead of using submenuId is defined test, we should define the variable before the if on line 47 ({% set submenuId = null %}) and then check if it is not empty ({% if submenuId is not empty %})
Contributor
Author
There was a problem hiding this comment.
Thanks Edi, I just fixed that
emodric
approved these changes
Jan 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This one is huge, sorry. The main purpose was to avoid opening menus on hover, have a consistent click/tap interaction, use proper elements for the submenu triggers, with proper ARIA attributes to comply with the best accessibility practices. Found few more issues along the way as usual.
Accessibility Improvements
<i>to semantic<button>elements with proper ARIA attributes (aria-haspopup,aria-expanded,aria-controls)aria-hiddenstate management and unique IDs for ARIA references.visually-hiddenclass (replaced deprecated.sr-onlyand custom.ttwhich was useless anyway)aria-hiddentoggle when navigation opens/closes, on page load set to trueUX Enhancements
max-height(utility functionsetSubmenuMaxHeightin JS)Code Refactoring
site-header-sticky--activetosite-header-sticky.scrolledno-triggersclass to disable interactive submenu triggers where not neededFiles Changed
PageHeader.component.js- Major refactor_navigation.scss- Animation and button trigger stylesmenu.html.twig- ARIA attributes and button triggersfooter.html.twig,search_box.html.twig,forms/theme.html.twig- Screen reader class updatesNotes
no-triggersclass, which effectively changes the child submenu-trigger to a plain heading for the subitems. This can be useful for the cases where the subitems should be displayed directly, and not in interactive menu (typically in the footer, based on my experience).isMobilebreakpoint in JS has to be kept in sync with the SCSS breakpoint variables, as I noted in the codescrolledclass is added to site header regardless its relative, fixed, or sticky position. This should be simpler and more universal solution, enabling easy styling if needed (e.g. styling fixed header differently if scrolled). And the BEM syntax wasn't use anywhere else, anyway