-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Accessible Progress #1089
Accessible Progress #1089
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/primer/primer-css/7u3agv9e7 |
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.
Implementation wise 👍🏻
I like the idea of the min-width on the sections, but I'd be curious to what @emilybrick thinks.
.Progress-item + .Progress-item { | ||
// stylelint-disable-next-line primer/spacing | ||
margin-left: 2px; | ||
} |
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 like how simple this solution is. If only we could do flex gap now. 😩
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 PR improves accessibility of the
Progress
component by adding a2px
gap between multiple values.Why
This makes it easier to see different values that have similar colors.
Alternatives
If there are a lot of very small values, like
0.1%
, they might not be visible and it just shows some empty space towards the end:An option would be to show every value with a
min-width
of2px
. So that they all show up. Downside it that it makes the values look different than they actually are, so for now this is not included in this PR.API changes
.Progress-item
class needs to be added for Progress with multiple valuesCloses #1079