-
Notifications
You must be signed in to change notification settings - Fork 378
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
a11y: fix NavItem a11y issues #2502
Conversation
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.
|
I changed the margin when active |
Thanks for taking care of that - this slipped my mind while I was working on the labels thing. |
That E2E failure is the exact same one I'm seeing on my other PR, too. The "SaveAs" task, which I haven't touched and which I was able to manually step through and it seems like it should work. |
I pulled this and tested locally. LGTM. Not sure about the failing test. @cwhitten is this related to the test failures you mentioned in our 1:1 yesterday? |
Yea @a-b-r-o-w-n is fixing that |
* fix NavItem a11y issues * Update styles.ts * fix NavItem a11y issues * Update styles.ts * Update styles.ts * move data-testid * Fix selected offset in left nav Co-authored-by: Andy Brown <asbrown002@gmail.com> Co-authored-by: Chris Whitten <christopher.whitten@microsoft.com>
Description
It was much easier to fix the problems in NavItem than to rip it out and replace it with a Nav (thanks to the still-unaddressed issues in Nav with SPA react-router-style links), so after fixing up the CSS and removing the unneeded button component, this was the result.
Task Item
Closes #2025
Closes #2040
Screenshots