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 issue with normal buttons with icons #7073

Merged
merged 3 commits into from Jun 1, 2018

Conversation

Projects
None yet
2 participants
@jasmussen
Contributor

jasmussen commented Jun 1, 2018

Reported in #5350 (comment), this PR fixes an issue where the icon was misaligned when using it with a standard Button look.

This scopes and adds some CSS to fix and align things. If this isn't the right approach, another approach would be to use the Button component here instead of the IconButton component (which has that no-background look). But then we would have to allow passing an icon to the Button component.

screen shot 2018-06-01 at 09 46 57

Fix issue with normal buttons with icons
Reported in #5350 (comment), this PR fixes an issue where the icon was misaligned when using it with a standard Button look.

This scopes and adds some CSS to fix and align things. If this isn't the right approach, another approach would be to use the Button component here instead of the IconButton component (which has that no-background look). But then we would have to allow passing an icon to the Button component.

@jasmussen jasmussen added the [Type] Bug label Jun 1, 2018

@jasmussen jasmussen self-assigned this Jun 1, 2018

@jasmussen jasmussen requested review from youknowriad and aduth Jun 1, 2018

&:hover,
&:focus:not(:disabled):not([aria-disabled="true"]) {
&:hover,
&:focus:not(:disabled):not([aria-disabled="true"]) {

This comment has been minimized.

@youknowriad

youknowriad Jun 1, 2018

Contributor

This is probably an unwanted change right?

This comment has been minimized.

@jasmussen

jasmussen Jun 1, 2018

Contributor

Yes indeed! Fix pushed.

@@ -26,7 +26,8 @@
box-shadow: inset 0 -1px 0 #cccccc;
vertical-align: top;
&:hover {
&:hover,
&.components-icon-button:not(:disabled):not([aria-disabled="true"]):hover {

This comment has been minimized.

@youknowriad

youknowriad Jun 1, 2018

Contributor

I feel it should be the opposite: avoid overriding the hover styles in components-icon-button's stylesheet if the button has the is-default className.

This comment has been minimized.

@jasmussen

jasmussen Jun 1, 2018

Contributor

So add something like a :not( .is-default ) there?

This comment has been minimized.

@youknowriad

youknowriad Jun 1, 2018

Contributor

yes, It's equivalent right? My idea is that the IconButton component uses the Button component which means it knows about its existence but not the opposite

This comment has been minimized.

@jasmussen

jasmussen Jun 1, 2018

Contributor

Should be fixed now!

jasmussen added some commits Jun 1, 2018

Address feedback.
Inverse the behavior for excluding normal buttons.
@@ -21,7 +21,7 @@
outline: none;
}
&:not( :disabled ):not( [aria-disabled="true"] ):hover {
&:not( :disabled ):not( [aria-disabled="true"] ):not( .is-default ):hover {

This comment has been minimized.

@youknowriad

youknowriad Jun 1, 2018

Contributor

Actually, now that I think about this more, I'm wondering why icon-buttons have different hover/active/disabled styles compared to the default styles they inherit from button component. Can't we just remove those overriddes? (Fine to address separately and get the fix in)

This comment has been minimized.

@jasmussen

jasmussen Jun 1, 2018

Contributor

This is because we are using aria-disabled and disabled to prevent the hover styles from being applied in some cases, like the up mover on the first block. Not sure there's a better fix.

This comment has been minimized.

@youknowriad

youknowriad Jun 1, 2018

Contributor

Yes, but why it's specific to icon buttons? Shouldn't we apply this to all buttons?

This comment has been minimized.

@jasmussen

jasmussen Jun 1, 2018

Contributor

Good point. Yes.

@youknowriad

LGTM 👍

@jasmussen jasmussen added this to the 3.0 milestone Jun 1, 2018

@jasmussen jasmussen merged commit b97eb13 into master Jun 1, 2018

2 checks passed

codecov/project 46.31% (-0.01%) compared to 17f1963
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jasmussen jasmussen deleted the fix/standard-icon-buttons branch Jun 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment