-
Notifications
You must be signed in to change notification settings - Fork 91
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
feat(button): add "featured" variant #1857
Conversation
✅ Deploy Preview for stacks ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
When filled/selected buttons were hovered, the currentColor would be used for the border color. This commit ensures the correct border color is used on filled/selected buttons
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.
Overall looks good, there is only a minor docs correction to do I left in comment.
Nice to see the PPCP abstraction making our lives easier here.
Well done @dancormier ❤️
Addresses #1857 (comment) and similar
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.
Ok so the only thing I don't love is that in HC mode, the filled default state is the same color as the filled selected state (no difference).
Because we had to bump the background color from 400 to 500 for HC filled, and because 500 and 600 are the same in HC, we end up with no difference there.
For Base and Danger we have a modified red-400 and blue-400 just for HC mode. Those colors were modified from Light/Dark so that they meet the 7:1 ratio (they're pretty close but not quite otherwise). And so for those variants we're able to keep the 400 stop as the background color for filled default and therefore have a color difference with the selected state.
So....
I'm proposing we modify HC modes purple 400 color to meet a 7:1 ratio with white.
Change the selected state back to using purple-400 but with this new color (in dark mode the purple 400 already passes 7:1 ratio.
- HC Light purple-400 would become #3B41BA (Figma where I played with this)
- HC Dark purple-400 would become NO CHANGE NEEDED? since it already passes the 7:1 ratio?
STACKS-680
TODO
Note
I've opened this PR up for review but there's a couple of outstanding items that need to be resolved:
This PR adds a "featured" (purple) variant to the button component.
Screenshots
Light
Dark
Light high contrast
Dark high contrast
Testing
/product/components/buttons/#featured
Notable changes
High contrast filled background color
To meet accessibility requirements, the featured filled variant background color has been set to--purple-500
(normally--purple-400
).We've modified the
purple-400
light HC mode color instead. See #1857 (review)Borders on filled, selected buttons
This PR includes a minor fix to button border colors when hovered and filled and/or selected. Previously, the value of
--bu-bc
(currentColor
) was applied and now the border color applied is equal to--bu-bg
, ensuring the border and button background are unified.Button group visual regression test images
There are six visual regression tests (all on
s-btn-group-highcontrast-*
in Chromium runs) that have been updated. They are visually identical to me but there could be extremely slight differences due to 838e25a.