Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/app/admin/AdminSidebar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

import { useState, useMemo } from 'react'
import { adminInteractive } from '@/lib/admin-ui'
import { navLinkClass } from '@/lib/design/nav'
import { navLinkProps } from '@/lib/design/nav'
import { useTranslations } from 'next-intl'
import Link from 'next/link'
import { Link as PublicLink } from '@/i18n/navigation'
Expand Down Expand Up @@ -213,7 +213,7 @@ export function AdminSidebar({
key={section.path}
href={section.path}
onClick={() => { setMobileMenuOpen(false); setFilterQuery('') }}
className={navLinkClass('sidebar', isActive(section.path))}
{...navLinkProps('sidebar', isActive(section.path))}
>
<Icon className="h-4 w-4 shrink-0 text-text-muted dark:text-text-secondary" />
<span className="flex-1 text-sm font-medium">
Expand Down Expand Up @@ -247,7 +247,7 @@ export function AdminSidebar({
<Link
href={only.path}
onClick={() => setMobileMenuOpen(false)}
className={navLinkClass('sidebar', onlyActive, sidebarCollapsed ? 'justify-center' : '')}
{...navLinkProps('sidebar', onlyActive, sidebarCollapsed ? 'justify-center' : '')}
title={sidebarCollapsed ? sectionLabel(tSections, only) : undefined}
>
<OnlyIcon className={`shrink-0 ${sidebarCollapsed ? 'h-5 w-5' : 'h-4 w-4'} ${onlyActive ? 'text-action' : 'text-text-muted dark:text-text-secondary'}`} />
Expand Down Expand Up @@ -290,7 +290,7 @@ export function AdminSidebar({
key={section.path}
href={section.path}
onClick={() => setMobileMenuOpen(false)}
className={navLinkClass('sidebar', active, sidebarCollapsed ? 'justify-center' : '')}
{...navLinkProps('sidebar', active, sidebarCollapsed ? 'justify-center' : '')}
title={sidebarCollapsed ? `${sectionLabel(tSections, section)}${sensitive ? ` (${t('sensitiveLabel')})` : ''}` : sensitivityReason}
>
{/* Larger icon when collapsed so it's easier to tap and recognise at a glance */}
Expand Down
46 changes: 46 additions & 0 deletions src/lib/design/__tests__/nav-link-props.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/**
* A nav link's styling and its announcement must come from the same call.
*
* `navLinkClass` returns only a className, so a caller could style the active
* item and forget to say which one it is — the highlight then exists only for
* people who can see it. AdminSidebar did exactly that on all three of its
* link sites while every other nav surface in this app set `aria-current`
* correctly. Four surfaces right and one wrong is what applying a rule by hand
* looks like at the margin, so `navLinkProps` returns both together and the two
* decisions cannot drift apart.
*/
import { navLinkClass, navLinkProps, NAV_STATE } from '@/lib/design/nav'

describe('navLinkProps', () => {
it('announces the active link as the current page', () => {
expect(navLinkProps('sidebar', true)['aria-current']).toBe('page')
})

it('leaves aria-current off inactive links rather than setting it false', () => {
// `aria-current="false"` is a valid token meaning "not current", but it is
// noise in the accessibility tree — omit the attribute instead.
expect(navLinkProps('sidebar', false)['aria-current']).toBeUndefined()
})

it('returns exactly the className navLinkClass would have', () => {
for (const shape of Object.keys(NAV_STATE) as (keyof typeof NAV_STATE)[]) {
for (const active of [true, false]) {
expect(navLinkProps(shape, active).className).toBe(navLinkClass(shape, active))
}
}
})

it('passes an extra className through unchanged', () => {
expect(navLinkProps('sidebar', true, 'justify-center').className).toBe(
navLinkClass('sidebar', true, 'justify-center'),
)
})

it('marks the active state for every nav shape, not just the sidebar', () => {
// The bug was one surface forgetting. A helper that only worked for the
// shape that happened to be broken would not close the class.
for (const shape of Object.keys(NAV_STATE) as (keyof typeof NAV_STATE)[]) {
expect(navLinkProps(shape, true)['aria-current']).toBe('page')
}
})
})
25 changes: 25 additions & 0 deletions src/lib/design/nav.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,3 +85,28 @@ export function navLinkClass(shape: NavShape, active: boolean, className?: strin
const s = NAV_STATE[shape]
return cn(s.base, active ? s.active : s.inactive, className)
}

/**
* The class AND the announcement, from one call.
*
* `navLinkClass` on its own lets a caller style the active item and forget to
* say which one it is — the highlight then exists only for people who can see
* it. AdminSidebar did exactly that on all three of its link sites while every
* other nav surface here set `aria-current` correctly, which is what applying
* a rule by hand looks like at the margin.
*
* Spread this instead of passing `className` by itself, and the two decisions
* cannot drift apart:
*
* <Link href={x} {...navLinkProps('sidebar', active)} />
*/
export function navLinkProps(
shape: NavShape,
active: boolean,
className?: string,
): { className: string; 'aria-current': 'page' | undefined } {
return {
className: navLinkClass(shape, active, className),
'aria-current': active ? 'page' : undefined,
}
}
Loading