-
Notifications
You must be signed in to change notification settings - Fork 842
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
[EuiButtonGroup] Improve interaction states #4142
[EuiButtonGroup] Improve interaction states #4142
Conversation
Preview documentation changes for this PR: https://eui.elastic.co/pr_4142/ |
Unfortunately this going to have a pretty big conflict with #4056 which is why it's been put into draft mode. We'll want to wait til that PR gets merged first as it completely re-works the styles because of the updated component structure. |
…mpressed-a11y-colors
…mpressed-a11y-colors
Preview documentation changes for this PR: https://eui.elastic.co/pr_4142/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_4142/ |
…mpressed-a11y-colors
Preview documentation changes for this PR: https://eui.elastic.co/pr_4142/ |
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.
Thanks for tackling this one. Sorry for the conflicts that surely ensued. I'm visually ok with more pronounced selected state, but it did create a few visual regressions.
Preview documentation changes for this PR: https://eui.elastic.co/pr_4142/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_4142/ |
…mpressed-a11y-colors
@cchaos thanks for your previous feedback. I decided to make all the interaction states for the EuiButtonGroup similar. The only difference is that the compressed buttons have a focus ring. The best way to test the changes is to replace the files Then in |
Preview documentation changes for this PR: https://eui.elastic.co/pr_4142/ |
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.
* Medium and Small sizing (regular button style) | ||
*/ | ||
|
||
// sass-lint:disable nesting-depth |
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'd guess this lint disable is not needed anymore?
@cchaos the I thought it didn't have enough contrast when the background gets darker: So I decided to use the Is it too much? 😛 |
Preview documentation changes for this PR: https://eui.elastic.co/pr_4142/ |
I'm thinking that in general the buttons are going too dark on hover. We should probably be consistent with our base buttons which only darken by |
Preview documentation changes for this PR: https://eui.elastic.co/pr_4142/ |
Summary
Closes #4105.
This PR improves the EuiButtonGroup interaction states.
Checklist
[ ] Checked in mobile[ ] Props have proper autodocs[ ] Added documentation[ ] Checked Code Sandbox works for the any docs examples[ ] Added or updated jest tests[ ] Checked for breaking changes and labeled appropriately