Skip to content
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

feat(button): add warning modifier to buttons #3349

Merged
merged 3 commits into from Aug 17, 2020

Conversation

jenny-s51
Copy link
Contributor

Closes #3251

@mcoker mcoker requested a review from mcarrano July 30, 2020 21:39
@patternfly-build
Copy link

patternfly-build commented Aug 4, 2020

Preview: https://patternfly-pr-3349.surge.sh

A11y report: https://patternfly-pr-3349-coverage.surge.sh

CSS Size Report
NameCurrentPreviousDiff %
components/Button/button.css22.7 kB21.2 kB6.74
patternfly.min.css686.5 kB685.1 kB0.20
patternfly-no-reset.css775.7 kB774.2 kB0.20
patternfly.css777.6 kB776.1 kB0.20

@mcoker mcoker added this to the 2020.11 milestone Aug 4, 2020
@mcoker mcoker requested a review from mceledonia August 4, 2020 21:55
// Warning btn
--pf-c-button--m-warning--BackgroundColor: var(--pf-global--warning-color--100);
--pf-c-button--m-warning--Color: var(--pf-global--Color--dark-100);
--pf-c-button--m-warning--hover--BackgroundColor: var(--pf-global--warning-color--200);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked with @mceledonia and he suggested using --pf-global--palette--gold-500 for hover/focus/active background color instead, and leaving the text color as it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you Michael! Sounds good -- I'll make this change.

@mcoker mcoker removed this from the 2020.11 milestone Aug 4, 2020
@jenny-s51 jenny-s51 requested a review from mcoker August 5, 2020 17:00
Copy link
Contributor

@mcoker mcoker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

woot! thanks!

@christiemolloy
Copy link
Member

christiemolloy commented Aug 17, 2020

Looks like there are multiple styles for the states for the warning button, which makes for a jarring experience. Most buttons have an initial state and their hover/focus are the same, should we also do that for the warning button?

The initial state is:
Screen Shot 2020-08-17 at 4 24 15 PM

The hover state is:
Screen Shot 2020-08-17 at 4 23 39 PM

The focus state is:
Screen Shot 2020-08-17 at 4 23 44 PM

Thoughts? @mceledonia @mcoker @jenny-s51

@mcarrano
Copy link
Member

Good catch @christiemolloy . Why would the label color invert on focus? Feels like a bug.

@mcoker
Copy link
Contributor

mcoker commented Aug 17, 2020

@christiemolloy good catch! I think the focus color is just an oversight. Confirmed with @mceledonia it should be the normal text color. @jenny-s51 when you get a chance, can you make that update?

@jenny-s51
Copy link
Contributor Author

Thoughts? @mceledonia @mcoker @jenny-s51

nice catch @christiemolloy! fixed 👍

Copy link
Member

@christiemolloy christiemolloy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for fixing! looks great!

@jenny-s51 jenny-s51 merged commit 03ddc4e into patternfly:master Aug 17, 2020
@jenny-s51 jenny-s51 deleted the iss3251 branch August 17, 2020 20:59
@redallen
Copy link
Contributor

🎉 This PR is included in version 4.32.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warning modifier added to buttons
7 participants