-
Notifications
You must be signed in to change notification settings - Fork 409
frontend: move to new sidebar #2130
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Changes Made:
1. SidebarLogo
- Added aria-label="Go to Overview" to the link
- Set alt="" on decorative image (aria-label describes the action)
2. SidebarCollapseToggle
- Added aria-expanded to indicate sidebar state
- Added aria-label describing the action
- Added aria-hidden="true" to icons
3. UserProfileNew
- Added aria-label to dropdown trigger (User menu for {name})
- Added aria-hidden="true" to all decorative icons (ChevronUp, Settings, LogOut)
- Set alt="" on avatar image (aria-label on button describes context)
4. SidebarNavItem
- Added aria-current="page" for active navigation items
- Added aria-disabled for disabled items
- Added aria-hidden="true" to nav icons
- Changed disabled item container from <div> to <span role="link"> for better semantics
5. SidebarNavigation
- Wrapped menu in <nav aria-label="Main navigation"> landmark
graham-rp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a lot to add without any additional test coverage. sidebar.tsx is also really hard to read
| // c could be null, because the call to /api/console/endpoints failed or is not responded yet, | ||
| // in that case those endpoints should be considered unsupported | ||
| switch (f.endpoint) { | ||
| case Feature.SchemaRegistryACLApi.endpoint: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: why change this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This actually meant we are able to accidentally let the user click into a sidebar item before we realized that 1 of the endpoints was failing, meaning the feature should be disabled because that service is not responsive.
|
Closing this in favor of #2132 |
I agree. I fixed it in #2132 |
Fix for:
Summary
Introduces a new modern sidebar for self-hosted Console, replacing the legacy Chakra UI implementation with Tailwind/shadcn components.
Changes
BEFORE



AFTER