-
Notifications
You must be signed in to change notification settings - Fork 100
feat(button) - update button base styles for SHINE #2008
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
base: beta
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 850fdf3 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
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.
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 🙂
| transition: opacity 200ms var(--te-smooth); // Animate the transition to .is-loading | ||
|
|
||
| &:not(:last-child) { | ||
| margin-right: var(--_bu-icon-gap, 8px); |
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.
No need for the fallback 8px value here. Since --_bu-icon-gap is defined in a broader scope, it should always have a value.
| } | ||
|
|
||
| &:not(:first-child) { | ||
| margin-left: var(--_bu-icon-gap, 8px); |
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.
Same here. No need for the fallback 8px value.
| --_bu-bc: transparent; | ||
| // --_bu-bg: inherit; // [1] | ||
| --_bu-br: var(--br-md); | ||
| --_bu-br: 20px; |
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.
Instead of using a hardcoded value here, I suggest using a border radius variable (I think --br-pill would be best).
Extra context: Design plans on simplifying our border radii to something like none (0), round (10px I think), pill (1000px), and potentially circle (100%).
I suggest adding a todo here to convert this value to an atomic value once border radius is updated in our libraries (see SPARK-91).
| --_bu-p: 10px; | ||
| --_bu-px: 16px; | ||
| --_bu-py: 10px; |
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.
| --_bu-p: 0.6em; | ||
| --_bu-fs: 12px; // Override size-styles for custom font size | ||
| --_bu-px: 12px; // 12px padding as per Figma specs | ||
| --_bu-py: 10px; | ||
| --_bu-icon-gap: 4px; // 4px spacing as per Figma specs |
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 sort of stuff will be tricky until we have base styles updates like typography figured out. You'll see a .size-styles mixin a couple of lines up. This mixin allows us apply the same styles for a given size across multiple components. We could modify that mixin to account for that but I'm leaning towards just adding a few todos to port things over to that mixin.
| --_bu-fs: 13px; // Override size-styles for custom font size | ||
| --_bu-px: 12px; // 12px padding as per Figma specs | ||
| --_bu-py: 10px; // Calculated to achieve 32px height: (32 - 12) / 2 = 10px | ||
| --_bu-icon-gap: 6px; // 6px spacing as per Figma specs |
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.
In order to achieve the desired button height with the suggested padding, we'll probably need to adjust line-height pretty significantly. For instance, the xs button can hit ~28px height with line-height: .5, but this is a very tight line height that would result in some rough display if it ever needs to wrap.
For the button sizes in general, I suggest asking via a comment on the Figma asking if the overall height is the goal over specific padding values. I suspect hitting ~28px height is more important. If that's true, I'd ignore the recommended padding values in favor of hitting the height while ensuring the text is vertically centered.
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.
Ignore padding specified in Figma and target height OR do recommendation above
| display: inline-block; | ||
| font-family: inherit; | ||
| font-weight: normal; | ||
| font-weight: var(--_bu-fw, 600); |
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.
No need for the fallback value here
|
Discussion in this slack thread
|
| --_bu-dropdown-bw: calc(var(--su-static4) - var(--su-static1)); | ||
| --_bu-p: 0.6em; | ||
| --_bu-p: var(--su12); | ||
| --_bu-py: 6.285px; // Adjust to reach 28px height |
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.
round 6.285 down or up
CGuindon
left a comment
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.
- 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.
|
I think we might need a call to talk through the selected state options — I'll set that up. |
| }); | ||
|
|
||
| .dark-mode({ | ||
| --_bu-filled-bg-selected: linear-gradient(to bottom, var(--theme-secondary-600) 0%, var(--theme-secondary-600) 50%, color-mix(in srgb, var(--theme-secondary-600) 80%, black 40%) 50%, color-mix(in srgb, var(--theme-secondary-600) 80%, black 40%) 100%); |
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.
@CGuindon by reversed I'm assuming you meant use black instead of white for the gradient instead of flipping the colors.
|
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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 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. |


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
NOTE: I haven't pushed the visual regression tests yet (there's over 2.5k) that's why the visual regression tests are failing
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