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?
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
In addition to the inline link disabled issue mentioned above, the contrast for the other disabled/aria-disabled buttons is failing in Firefox for both light and dark themes due to the background 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. |
andrew-ronaldson
left a comment
andrew-ronaldson
left a comment
There was a problem hiding this comment.
Loving the buttons! The only thing that I noticed is the icon button. I remember talking to Lucia about a min-width to match the height so it would be more squared off.
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
✅ Had a comment below, but not really blocking. The above comment regarding contrasts could also be a followup.
| --#{$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. |
srambach
left a comment
srambach
left a comment
There was a problem hiding this comment.
Aside from Andrew's question about min-width and Eric's about the progress spinner color, everything looks good to me. 🚨
|
@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 
|
srambach
left a comment
srambach
left a comment
There was a problem hiding this comment.
I love the way you've used the fallbacks on the inline-link. ✨
|
🎉 This PR is included in version 6.0.0-alpha.43 🎉 The release is available on: Your semantic-release bot 📦🚀 |
5 participants
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