Conversation
github-project-automation
bot
moved this to 🏗 In progress
in UX Engineering Team Backlog
mfranzke
moved this from 🏗 In progress
to 🎁 Ready for review
in UX Engineering Team Backlog
There was a problem hiding this comment.
Pull request overview
This pull request refactors the disabled link handling in navigation items by moving accessibility attributes from the DBNavigationItem component to the inner <a> element. The changes add tabindex and aria-disabled attributes to anchor tags when links are disabled.
Changes:
- Moved
aria-disabledattribute fromDBNavigationItemto the child<a>element - Added
tabindex="-1"to the<a>element when disabled to prevent keyboard navigation
| <a | ||
| href={trimExtension(target)} | ||
| aria-current={isActive ? 'page' : undefined} | ||
| data-icon-trailing={iconTrailing} | ||
| tabindex={disabled ? '-1' : undefined} | ||
| aria-disabled={disabled ? 'true' : undefined} | ||
| > |
There was a problem hiding this comment.
When a link is disabled, it should not be navigable via click. Consider adding an onClick handler that prevents navigation when disabled, or use CSS pointer-events to prevent interaction. Without this, users can still navigate to the disabled link by clicking on it, defeating the purpose of disabling it.
There was a problem hiding this comment.
Copilot is right. Just adding aria-disabled and tabIndex doesn't prevent clicks from triggering navigation. One fix is to conditionally remove the href when disabled (e.g., href={disabled ? undefined : path}).
| <a | ||
| href={trimExtension(path)} | ||
| aria-current={getAriaCurrent(trimExtension(path))} | ||
| data-icon-trailing={iconTrailing} | ||
| tabindex={disabled ? '-1' : undefined} | ||
| aria-disabled={disabled ? 'true' : undefined} | ||
| > |
There was a problem hiding this comment.
When a link is disabled, it should not be navigable via click. Consider adding an onClick handler that prevents navigation when disabled, or use CSS pointer-events to prevent interaction. Without this, users can still navigate to the disabled link by clicking on it, defeating the purpose of disabling it.
| href={trimExtension(target)} | ||
| aria-current={isActive ? 'page' : undefined} | ||
| data-icon-trailing={iconTrailing} | ||
| tabindex={disabled ? '-1' : undefined} |
Copilot
AI
Jan 29, 2026
•
There was a problem hiding this comment.
| tabindex={disabled ? '-1' : undefined} | |
| tabIndex={disabled ? '-1' : undefined} |
In React/JSX, the attribute should be tabIndex (camelCase) instead of tabindex (lowercase). React will issue a warning about this in the console, and the lowercase version may not work correctly in all scenarios.
| href={trimExtension(path)} | ||
| aria-current={getAriaCurrent(trimExtension(path))} | ||
| data-icon-trailing={iconTrailing} | ||
| tabindex={disabled ? '-1' : undefined} |
Copilot
AI
Jan 29, 2026
•
There was a problem hiding this comment.
| tabindex={disabled ? '-1' : undefined} | |
| tabIndex={disabled ? '-1' : undefined} |
In React/JSX, the attribute should be tabIndex (camelCase) instead of tabindex (lowercase). React will issue a warning about this in the console, and the lowercase version may not work correctly in all scenarios.
mfranzke
moved this from 🎁 Ready for review
to 🏗 In progress
in UX Engineering Team Backlog
michaelmkraus
changed the title
mfranzke
changed the title
Correctly "disabling" links in db-nav-item components. We shouldn't disable a non-interactive, purely structural element (list-item), but the links directly.