feat: use Navi for primary navigation#34
Conversation
WybeBosch
requested review from
FreakyWizard,
JasperS20,
Rovasch,
SimonvanWijhe,
Yannicvanveen,
YvetteNikolov,
dtakken,
hnccox-yard,
ictbeheer,
laravdiemen,
mvdhoek1 and
rivanuff
and removed request for
a team
| @foreach ($item->children as $child) | ||
| <li class="menu-item"> | ||
| <a href="{{ $child->url }}" @class([ | ||
| 'group/child flex items-center px-6 py-3 text-left leading-snug text-inherit no-underline', |
There was a problem hiding this comment.
Deze group/child mag weg?
|
Je moet nog:
|
| {{ $item->label }} | ||
|
|
||
| @if ($item->children) | ||
| <i class="fa-light fa-chevron-down pl-2"></i> |
There was a problem hiding this comment.
Hier kunnen we nu een SVG van maken! Net zoals in de search bar. Scheelt weer zo'n layout shift,
| <a href="{{ $item->url }}" @class([ | ||
| 'relative flex h-full items-center px-2 text-center text-sm leading-snug text-black no-underline xl:px-4 xl:text-base xl:leading-snug hocus:text-primary', | ||
| 'text-primary' => $item->active || $item->activeParent, | ||
| $item->classes, |
There was a problem hiding this comment.
aria-current toevoegen als het active is. Check sowieso even of je nog andere aria attributen mist met acceptatie
WybeBosch
force-pushed
the
feat/add-navi-for-navigation-menu
branch
from
cc012b8 to
3da8f63
Compare
Composer package changes
|
| @@ -1,13 +1,50 @@ | |||
| @if (has_nav_menu('primary_navigation')) | |||
| @php | |||
| $menu = Navi::build('primary_navigation'); | |||
There was a problem hiding this comment.
Ik weet niet waarom dit werkt, maar volgens de documentatie roep je eerst de static method make aan en vervolgens build. PHPStorm geeft momenteel aan dat Navi undefined is. Als je deze importeert krijg je de error dat build niet static is.
Uit de docs:
use Log1x\Navi\Navi;
$menu = Navi::make()->build('primary_navigation');|
|
||
| @if ($item->children) | ||
| <ul | ||
| class="sub-menu ease-base invisible absolute mb-0 min-w-48 -translate-y-3 list-none bg-white pl-0 opacity-0 shadow-md transition-all group-[.js-brave-show-sub-menu]:visible group-[.js-brave-show-sub-menu]:translate-y-0 group-[.js-brave-show-sub-menu]:opacity-100"> |
There was a problem hiding this comment.
Zolang de JS ervoor zorgt dat aria-expanded bijgewerkt wordt bij alle mogelijke manieren om de uitklap te openen kunnen we de styling daar ook op baseren. group-[.js-brave-show-sub-menu] zou dan dus group-has-aria-expanded kunnen zijn. De toepassing van aria-attributen op dit soort componenten zal minder onderhevig zijn aan veranderingen dan class namen. Eventueel zou de groep nog een naam kunnen krijgen om iets duidelijker te maken om welke groep het gaat indien er meerdere zijn.
YvetteNikolov
force-pushed
the
feat/add-navi-for-navigation-menu
branch
from
a60388b to
e678b13
Compare
YvetteNikolov
force-pushed
the
feat/add-navi-for-navigation-menu
branch
3 times, most recently
from
3acd7c0 to
cbd2c25
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
YvetteNikolov
force-pushed
the
feat/add-navi-for-navigation-menu
branch
from
d6dc589 to
adf7d9d
Compare
| <path | ||
| d="M235.3 411.3c-6.2 6.2-16.4 6.2-22.6 0l-208-208c-6.2-6.2-6.2-16.4 0-22.6s16.4-6.2 22.6 0L224 377.4 420.7 180.7c6.2-6.2 16.4-6.2 22.6 0s6.2 16.4 0 22.6l-208 208z" /> | ||
| </svg> | ||
| @endif | ||
|
|
||
| <span @class([ | ||
| 'bg-primary ease-base absolute bottom-0 w-full left-0 h-1 duration-300 invisible scale-0 group-hover:visible group-hover:scale-100', | ||
| 'visible scale-100' => $item->active || $item->activeParent, |
There was a problem hiding this comment.
🔴 The Navi-based navigation replaces WordPress's wp_nav_menu(), which automatically added aria-current="page" to active links since WP 5.0+, but all three new templates (navigation.blade.php, mobile-menu.blade.php, top-bar.blade.php) only pass activeClass for CSS styling and never bind aria-current. Without aria-current="page", screen reader users cannot determine which link corresponds to the current page. Fix: add :aria-current="$item->active ? 'page' : false" to each x-brave::nav.link call across all three templates.
Extended reasoning...
What the bug is and how it manifests
All three Navi-based nav templates introduced in this PR — navigation.blade.php, mobile-menu.blade.php, and top-bar.blade.php — use the activeClass prop on x-brave::nav.link to visually highlight the active menu item via CSS classes (e.g. text-primary, font-bold), but none of them pass an aria-current attribute to the rendered <a> element. Screen readers rely on aria-current="page" to announce the current page link when a user navigates through a navigation landmark. Without it, assistive technology users hear no indication of which link is the current page.
The specific code path that triggers it
In navigation.blade.php (line 21), mobile-menu.blade.php (line 47 and 57), and top-bar.blade.php (line 16), x-brave::nav.link receives :item and activeClass props but receives no :aria-current binding. The activeClass prop causes the component to add CSS classes when $item->active is true, but there is no built-in mechanism in the component to automatically translate this into an aria-current attribute — as confirmed directly by PR reviewer YvetteNikolov's comment on 2026-01-30: 'aria-current toevoegen als het active is.' ('add aria-current when it is active'), which implies the component does not handle this automatically.
Why existing code doesn't prevent it
The old wp_nav_menu() implementation was replaced entirely. WordPress core's Walker_Nav_Menu::start_el() has automatically added aria-current="page" to the active menu item since WordPress 5.0, so the previous rendering path provided this accessibility signal for free. The new Blade templates rendering through x-brave::nav.link bypass the WordPress nav walker entirely and receive no equivalent treatment. A grep of the entire codebase shows zero occurrences of aria-current anywhere in the theme templates.
What the impact would be
Screen reader users navigating the main menu, mobile menu, or top bar menu cannot determine which link corresponds to the page they are currently viewing. WCAG 2.1 technique ARIA5 and SC 1.3.1 (Info and Relationships) recommend aria-current="page" on the current-page nav link within navigation landmarks. This is a direct regression from the previous WordPress-based implementation which provided this signal automatically.
How to fix it
Add :aria-current="$item->active ? 'page' : false" to each x-brave::nav.link invocation in all three templates. Passing false (rather than omitting the attribute) ensures the attribute is not rendered on inactive links. For child items, use the same pattern with $child->active.
Step-by-step proof
- A site editor creates a primary navigation menu with three items: Home, About, Contact. The user is on the About page, so Navi sets
$item->active = truefor the About item. - The About
x-brave::nav.linkreceivesactiveClass="text-primary"and renders an<a class="... text-primary" href="/about">About</a>— the CSS class is applied correctly. - However, the
<a>element has noaria-currentattribute at all. A screen reader user tabbing through the nav hears 'Home link', 'About link', 'Contact link' — identical for all three. There is no way for the user to know they are already on the About page. - In the old implementation,
wp_nav_menu()would render<a aria-current="page" href="/about">About</a>, and the screen reader would announce 'About, current page, link'. - The fix
:aria-current="$item->active ? 'page' : false"restores this behavior: the rendered HTML becomes<a aria-current="page" href="/about">About</a>for the active item only.
Edit Yvette: ready for review
Navi geïmplementeerd voor:
Na akkoord squash ik alle commits naar één,
draft
maar ACF is niet opensource / free volgens mij.