Skip to content
This repository has been archived by the owner on Jan 26, 2022. It is now read-only.

Add button hover styles #162

Closed
wants to merge 3 commits into from
Closed

Add button hover styles #162

wants to merge 3 commits into from

Conversation

jffng
Copy link
Collaborator

@jffng jffng commented Oct 25, 2021

Description

This is the simplest way I can think of to achieve the design target for button hover styles. I'm not sure it is a good idea to add them though, because strange combinations can occur if the user makes button color customizations at a global level. In other words, if I change the background or text color of buttons globally, these hover styles will still apply. The same is true of #163.

Screenshots

Kapture.2021-10-26.at.15.07.09.mp4

Testing Instructions

  • Add a button, verify the hover styles apply
  • Change the button padding globally and in a post, verify the hover styles apply correctly

@jffng jffng linked an issue Oct 25, 2021 that may be closed by this pull request
@carolinan
Copy link
Collaborator

Have you seen the post @aristath wrote about how to load the block styles only when the block is used?
https://make.wordpress.org/core/2021/07/01/block-styles-loading-enhancements-in-wordpress-5-8/
I think that would be a good method to use.

Question
Of the custom styles in theme.json, I think only the hover style needs to be custom, the rest could be under
blocks.button or blocks.button.elements.links? I am only wondering why this was preferred.

@carolinan
Copy link
Collaborator

(It would be good to document implementation choices, it would make a good article and future reference.)

@jffng jffng marked this pull request as ready for review October 26, 2021 19:18
@jffng
Copy link
Collaborator Author

jffng commented Oct 26, 2021

I think that would be a good method to use.

👍

Of the custom styles in theme.json, I think only the hover style needs to be custom, the rest could be under
blocks.button or blocks.button.elements.links? I am only wondering why this was preferred.

Good question. I removed typography and some other custom styles that were not needed. The rest are placed under custom because the CSS needs to access the values, to maintain consistent spacing / dimensions across hover and non-hover states, default vs outline vs color customized buttons. If they are placed under styles.blocks.button, the styles are compiled (no CSS variable is created) and added to the class targeting the button. Also the styles are applied to all buttons (regardless of whether they are the outline style or have color customizations), and in the case of borders and colors, we don't actually want that.

@kjellr
Copy link
Collaborator

kjellr commented Oct 26, 2021

@jffng how much of this approach (if anything) do you think could be migrated to Gutenberg. Maybe even as something that gets opted into?

@jffng
Copy link
Collaborator Author

jffng commented Oct 26, 2021

how much of this approach (if anything) do you think could be migrated to Gutenberg.

Ideally all of it, eventually?

We might be able to start by just adding the ability to set the background color on hover in Gutenberg.

@carolinan
Copy link
Collaborator

Unpopular opinion: unregister the outline style 🤐

@kjellr
Copy link
Collaborator

kjellr commented Oct 27, 2021

Just noting that in on trunk, the outline buttons have black borders and text. But in this PR, the border is green while the text is black.

(Neither of those seems correct to me — I think the outline should use green for both the text and border by default.)

@kjellr
Copy link
Collaborator

kjellr commented Dec 7, 2021

Closing in favor of #275.

@kjellr kjellr closed this Dec 7, 2021
@kjellr kjellr deleted the try/button-styles branch December 7, 2021 20:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Buttons need hover, active, and focus states
3 participants