-
Notifications
You must be signed in to change notification settings - Fork 841
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
Fixed a bug with EuiButtonEmpty
disabled state
#561
Fixed a bug with EuiButtonEmpty
disabled state
#561
Conversation
@@ -45,8 +45,11 @@ | |||
fill: $euiButtonColorDisabled; | |||
} | |||
|
|||
&:focus { | |||
background-color: transparent; |
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.
Keeping this style because I presume it was used to negate https://github.com/elastic/eui/pull/561/files#diff-f844f303cef56a189e1b5e25742bd388L74, but copy pasted from the original buttons, which do have a background color. Here the default background color is transparent
, so I'm using that instead.
As a housekeeping item for thought, I'm wondering if some of this should move to variables to make the different button stylesheets similar
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 would think you'd want it on :hover
as well. So maybe just replace the original background-color: $euiColorEmptyShade;
with transparent
?
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.
background-color: transparent;
is already in the base class, why would we repeat it for :hover
as well? Wouldn't that just make it harder for consumers to change the background color by hand?
@@ -45,8 +45,11 @@ | |||
fill: $euiButtonColorDisabled; | |||
} | |||
|
|||
&:focus { | |||
background-color: transparent; |
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 would think you'd want it on :hover
as well. So maybe just replace the original background-color: $euiColorEmptyShade;
with transparent
?
@@ -75,8 +78,6 @@ $buttonTypes: ( | |||
} | |||
|
|||
&:hover { | |||
background-color: transparent; |
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 wouldn't remove it from here. I think we only add the background color on :focus mainly for keyboard users.
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.
When the background is already white, like on the demo page, there's no noticeable difference at all though?
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 was just looking at these particular code snippets and not at it as a whole. So you're good.
1519637
to
2a7b751
Compare
Fixed a bug where
EuiButtonEmpty
would offer a white background on hover when it was disabled, even when there was no such background transition on hover when the buttons are not disabled.