feat(button) - update button base styles for SHINE #2008
Conversation
🦋 Changeset detectedLatest commit: 290432f The changes in this PR will be included in the next version bump. 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 |
✅ Deploy Preview for stacks ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for stacks-svelte ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
dancormier
left a comment
dancormier
left a comment
There was a problem hiding this comment.
Hey @ttaylor-stack I looked it over and made a few broad suggestions that might help out. I'm happy to discuss more while we pair on Monday. Thanks for taking on this beastly redesign 🙂
|
Discussion in this slack thread
|
ttaylor-stack
changed the title
CGuindon
left a comment
CGuindon
left a comment
There was a problem hiding this comment.
- We're going to keep the focus ring Black on all components so it won't match the featured and danger state button colors. Sorry I thought this was handled by the focus ring component we have (currently blue) and not the buttons. Just updated the figma file to reflect this — buttons in the Figma were out of date from this decision. We're keeping existing logic that the focus is always the same color — black-600.
s-btnands-btn__tonalshouldn't get a background fill for the focus state (one is getting black fill so the text disappears and the other is getting a light fill that makes it look tonal). We should just have the outer ring appear (making it look like an outlined button) — same as.s-btn .s-btn__dangerbut black.
- The selected state on the clear buttons seem to be missing the gradient/two-toned styling? And the
tonalselected button state has a gradient that looks much less noticeable than Figma (I'm not sure how to grab the values from the browser). Bottom grey in Figma is HLS 220, 8, 93 and top darker grey is black-200 (HLS 216, 8, 88).
- Can we try to get the default button size to match closer to 40px in height? By reducing the top/bottom padding a bit. I know the other ones aren't exact but it's over 41px right now. This will be a component that gets aligned with other components often and I've made other elements match the 40px height as well so they all line up nice.
.s-btn .s-btn__filledin dark mode doesn't have the two toned/gradient selected state. Is there a way to use color stops to make this gradient or is it using hard coded values? Looping in @dancormier for thoughts on how to approach this. Ideally we use the color stop values so we don't need to update this in the future if we change any color values.
.s-btn .s-btn__featured .s-btn__filledalso seems very low contrast between the two tones. That might be dark mode colors. Wondering what approach you took to making all these gradients now.
|
@ttaylor-stack I'll have a separate ticket for button badges — so leaving the change you made for HC mode is fine and great for now in this PR. |
There was a problem hiding this comment.
- Tonal seems to have lost it's tone. The default (non-selected) state should have a background color of
black-150. We don't have an instance of tonal__clear the tonal__filled is the only version. So filled should appear as the default fors-btn .s-btn__tonal
I think it's because it's pulling from theme-secondary. Wondering if for the tonal styles we need to stick to the black color stops — @dancormier thoughts?
For the |
|
For dark mode, I think the gradients are using the wrong white or black value or ordering in some cases. I mocked them up in Figma and added notes for what I changed on the gradient where it differs from the Light mode. Ultimatley, all of the selected states should have the darker tone on the top layer and light tone on the bottom layer. I'm not sure how the figma gradient values correlate to the code values but hoping this helps. |
dancormier
left a comment
dancormier
left a comment
There was a problem hiding this comment.
This PR is a beast and in order to keep things moving, I took the liberty to refactor the Less in this PR: #2052
@ttaylor-stack I'd like to take a little time to discuss the PR in our 1:1 and see if it's appropriate to merge into yours. I think we can save ourselves some future pain by structuring the Less and the custom properties a bit differently.
* Button styles refactor * minor tweaks * Styles and such
PR to update button component to match the new SHINE base styles
Ticket: https://stackoverflow.atlassian.net/browse/SPARK-55
Figma: https://www.figma.com/design/do4Ug0Yws8xCfRjHe9cJfZ/Project-SHINE---Product-UI?node-id=4272-20325&m=dev
Questions/Concerns:
red-500within the danger variant in high contrast mode in order to keep the 7:1 ratio. Let me know if that is okay or needs updating cc: @CGuindon @dancormier