-
Notifications
You must be signed in to change notification settings - Fork 97
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
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 |
eb3721b
to
12eb114
Compare
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.
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?
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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.
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.
✅ Had a comment below, but not really blocking. The above comment regarding contrasts could also be a followup.
--#{$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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
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.
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 |
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.
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 📦🚀 |
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