fix(a11y): Add bold font to selected nav items #1894
Conversation
✅ Deploy Preview for stacks ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
@qheaden-stack thank you for taking this on! I neglected to mention that the data-text approach requires the addition of a child element to contain the text and the :before pseudo-element (I actually totally forgot this) like in the button group buttons.
In any case, I've added a commit* that adds this element (.s-navigation--item-text) and shuffles around the Less as needed to support it. I'm happy to go over these changes with you if you'd like more detail. Feel free to change anything you see fit. Once this is ready for review, I'm happy to do so but I'd also include Giamir since I made a bunch of changes.
What's needed before merging
- Updated Navigation component documentation describing
data-text(see button group) - Updated visual regression images (I haven't updated the visual regression images since I'm not 100% that the design won't get modified)
- Guidance from design about width increase due to this change (see fix(a11y): Add bold font to selected nav items #1894 (comment))
Sidenote: I just want to say thanks for picking up these a11y issues. You've been one of the few I've seen picking up issues (I know not everyone has enthusiasm and/or bandwidth for improving accessibility) and I'm glad you make the time and effort to improve our accessibility ❤️
*you can safe ignore my first two commits in this PR. I missed that all my changes included a change to line endings that created changes on every line. The last commit I made includes my changes without this line changes
|
@CGuindon I wanted to raise a design concern with you and see how/if we want to address it. With the addition of the hidden bold text to maintain a consistent size for navigation items between selected states, the items are now all a bit wider: Before (361px wide)
After (378px wide)
This has two potential issues:
This issue could potentially be partially mitigated by reducing the gap between navigation items: With gap reduced from 4px to 1px (366px wide)
The compromise is that hover state reveals that the navigation items are closer together. I'm happy to brainstorm other ideas or just leave it as-is if that seems right. |
|
@dancormier I don't think we can reduce the spacing to 1px — I believe this might cause issues with the focus state (the blue ring adds 1-2 px on the outside sometimes?). I'm honestly not too worried about the navigation get a tiny bit wider. Navigations are already wrapping for responsive screen sizes — I'd be curious to first see if there are any pages where the nav perfectly fits right now and it becomes a big issue for a regular desktop size. |
This reverts commit 6105021.
This time without all the silly line ending noise!
Not sure how that happened originally 🤷♂️
qheaden-stack
force-pushed
the
qheaden/a11y/navigation-component-updates
branch
from
56a606c to
f89a02e
Compare
🦋 Changeset detectedLatest commit: d0fc433 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
qheaden-stack
changed the title
There was a problem hiding this comment.
The changes made sense to me. Thanks for picking this up.
Before merging we need to add a changeset.
Also it would be nice to know ahead of time if there is something special we need to watch out for when performing the upgrade in Core. If no side effects are expected we can also cut a release straight away.
|
Note: This change is ready but we're delaying merging until prep work has been completed in Core. |
STACKS-755, Partially resolves A11Y-15