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

Update mixin that sets hover style on some buttons #17581

Merged
merged 2 commits into from Oct 8, 2019

Conversation

@enriquesanchez
Copy link
Contributor

commented Sep 25, 2019

Fixes #17337

This is a small update to the button-style__hover mixin in _mixins.scss. The change makes the hover style match the "hard line" style used on the block toolbar and the "Settings" and "More tools & options".

hovers

Some nitpicks

This change makes more noticeable the size/padding difference between the top toolbar and block toolbar buttons that was already present. Not sure if it's worth looking at this, thoughts?

toolbar-hovers

We also have another hover style in the block inserter, should we also look at updating this one?

Screen Shot 2019-09-25 at 1 27 47 PM

@mapk

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2019

I really like the consistency here, @enriquesanchez! Thanks for taking this on!

I also noticed that this PR updates these as well, which is excellent.

icons

@mapk

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2019

It also updated it here too:

toptoolbar

One thing to note for another PR is the "Block Navigation" popover hover state AND the block's "More Options" popover hover state don't match the rest of the popover hover states. We'll need to create another PR for this.

... that is... if we want this hover state to perpetuate into the popover menus. After looking at it more, I'm inclined to keep the grey solid hover states for the popover menus. It's great on the icons though.

@enriquesanchez

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2019

Thanks for the feedback @mapk!

I've created another PR (#17621) to address the hover styles of the dropdown menus.

@karmatosed

This comment has been minimized.

Copy link
Member

commented Sep 27, 2019

Should buttons have the style that is coming from core over the hardline? Specifically, the publish button.

@enriquesanchez

This comment has been minimized.

Copy link
Contributor Author

commented Oct 1, 2019

@karmatosed I'm not sure. Think that can be explored on another issue? Particularly if it refers to the Publish button.

@enriquesanchez

This comment has been minimized.

Copy link
Contributor Author

commented Oct 1, 2019

@mapk I added the styles to match the updates from #17621, which has already been merged. Would you mind giving it a second look just to be safe?

@swissspidy swissspidy changed the title Update mixing that sets hover style on some buttons Update mixin that sets hover style on some buttons Oct 2, 2019
@mapk
mapk approved these changes Oct 3, 2019
Copy link
Contributor

left a comment

Just tested again, @enriquesanchez and it works really well. It has removed the original hover effect which would have been odd keeping and now shows the lighter grey. Thanks! 👍

hover

@gziolo gziolo merged commit 7110f85 into master Oct 8, 2019
4 of 7 checks passed
4 of 7 checks passed
pull-request-automation
Details
Header rules - gutenberg-playground No header rules processed
Details
Pages changed - gutenberg-playground 5 new files uploaded
Details
Redirect rules - gutenberg-playground No redirect rules processed
Details
Mixed content - gutenberg-playground No mixed content detected
Details
Travis CI - Pull Request Build Passed
Details
netlify/gutenberg-playground/deploy-preview Deploy preview ready!
Details
@gziolo gziolo deleted the update/match-hover-state-of-top-toolbar-buttons branch Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.