Spark 55/shine update button base styles #2008
Conversation
* Optimize webpack files * upgrade webpack config files to ECMA script * Delete webpack-common.js * fix incorrect extraction of docs.css for stacks docs --------- Co-authored-by: Giamir Buoncristiani <giamir.buoncristiani@gmail.com>
🦋 Changeset detectedLatest commit: 775b57e 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. |
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 🙂
| 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.
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.
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.
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.
| --_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.
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.
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.
Example images
line-height: .5 single line
line-height: .5 multi-line
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.
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.
No need for the fallback value here
2 participants
No description provided.