fix(buttons): add tokens #6127
fix(buttons): add tokens #6127
Conversation
|
Preview: https://patternfly-pr-6127.surge.sh A11y report: https://patternfly-pr-6127-a11y.surge.sh |
mcoker
force-pushed
the
issue-5998-2
branch
2 times, most recently
from
eb3721b
to
12eb114
Compare
There was a problem hiding this comment.
Couple of quick questions just from the preview:
- Do we want any space around the inline link for the focus ring? It's really tight.
- Inline disabled links aren't legible in dark theme - I think it's ultimately getting the on-disabled color rather than the disabled text color?
|
|
||
| .#{$button} { | ||
| // Component | ||
| :root { |
There was a problem hiding this comment.
Missing the component selector here
|
@srambach good catch, updated the disabled inline link text/icon colors and added a small outline-offset. @thatblindgeye I'll leave that for @andrew-ronaldson and @lboehling. As long as the usage of vars all looks correct, I'd say the component should be fine to merge even if we update the contrast of those colors later. |
| --#{$button}--m-progress--TransitionProperty: padding; | ||
| --#{$button}--m-progress--TransitionDuration: var(--#{$pf-global}--TransitionDuration); | ||
| --#{$button}--m-progress--PaddingRight: calc(var(--pf-t--global--spacer--md) + var(--#{$button}__progress--width) / 2); | ||
| --#{$button}--m-progress--PaddingLeft: calc(var(--pf-t--global--spacer--md) + var(--#{$button}__progress--width) / 2); | ||
| --#{$button}--m-in-progress--PaddingRight: var(--pf-t--global--spacer--md); | ||
| --#{$button}--m-in-progress--PaddingLeft: calc(var(--pf-t--global--spacer--md) + var(--#{$button}__progress--width)); | ||
| --#{$button}--m-in-progress--m-plain--Color: var(--pf-t--global--color--brand--default); | ||
| --#{$button}--m-in-progress--m-plain--Color: var(--#{$spinner}--Color); |
There was a problem hiding this comment.
Nit: do we want to rely on Spinner's styles here since the other progress button's don't? Just wondering if it'd be confusing that a plain progress button's spinner color would change when the spinner Color var is updated, but none of the other progress button's do. May also be the case that updating the progress--Color var would change the other progress button's spinner color, but not the plain progress button.
There was a problem hiding this comment.
@thatblindgeye great question, I just went with the best I could tell based off of the designs.
@andrew-ronaldson per the figma designs, the spinners in progress buttons look to have the same color as an icon in that button would have. So for example, the spinner component in figma uses the color-brand-default token, but in a link button, the spinner uses icon-color-brand-default, which is the same color that an icon in a link button uses.
In v5, we support spinners in plain buttons and the spinner color is just whatever the spinner component color is. So for that support in penta, I'm setting the plain button's spinner color to the spinner default color (color-brand-default). WDYT? I didn't see a plain progress button in figma, though I could have overlooked it if you know of one.
I'd second this for the purposes of target size. |
|
@andrew-ronaldson updated! Right now the min-width is set to this math formula based off of the plain button's styles:
|
|
@srambach @thatblindgeye updated the min-width, control variation left/right padding, and confirmed with @andrew-ronaldson the plain spinner color should come from the spinner component (we don't want to set it to anything). |
|
@srambach updated the inline link text properties to inherit. Here's an inline link button in a title component 
|
|
🎉 This PR is included in version 6.0.0-alpha.43 🎉 The release is available on: Your semantic-release bot 📦🚀 |
fixes #5998
I updated the examples page to (hopefully) make it easier to see the different variations - I'll back those updates out prior to merge