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

[TRY] Button Block Variations #101

Closed
wants to merge 1 commit into from
Closed

Conversation

GeoffDusome
Copy link
Contributor

What does this do/fix?

Adding most of you for visibility on this change. I definitely like this approach for the button block specifically as it allows us to get away from having that "Default" style on the block we really didn't want to have and forcing us to find ways to style that "Default" when it didn't even get a class.

  • remove all block styles from the button block
  • add 3 new block variations for the button block (Primary, Secondary, Ghost)
  • this allows us to actually remove the "Default" block style (so we aren't stuck with an extra button we don't want), while still maintaining our current style system for the button block using 3 classes (is-style-primary, is-style-secondary and is-style-ghost).
  • update styleguide pattern with new buttons

QA

Screenshots/video:

Copy link
Contributor

@ChrisMKindred ChrisMKindred left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the difference between Varient and Style in this case? Aren't they doing the same thing in this use case? Couldn't we have just set is-default = true on the primary style?

@GeoffDusome
Copy link
Contributor Author

What is the difference between Varient and Style in this case? Aren't they doing the exact same thing in this use case?

Correct, yeah. The only difference in using block variants here instead of block styles is that we can actually remove the "Default" button block style. Which is a style of button that only inherits values from theme.json and is only targetable through extra styling by chaining :not() selectors to target the rest of the block styles you're defining. This allows us only to have the 3 styles we've defined in our styleguide and nothing more. I would say it just slightly improves UX.

@LayaTaal
Copy link
Contributor

LayaTaal commented Sep 7, 2023

@GeoffDusome one of the issues we've run into on Comic Con is styling buttons outside of the buttons block. For instance form submit buttons, file download block, etc.

The way we are styling them now, I ended up having to copy/paste a bunch of CSS properties over to style those correctly. We also don't pick up any of the styles from theme.json automatically.

I'm curious if you see this method helping to improve that at all or if just reworking how we apply button styles a bit is an equally good solution.

@GeoffDusome
Copy link
Contributor Author

GeoffDusome commented Sep 7, 2023

@GeoffDusome one of the issues we've run into on Comic Con is styling buttons outside of the buttons block. For instance form submit buttons, file download block, etc.

The way we are styling them now, I ended up having to copy/paste a bunch of CSS properties over to style those correctly. We also don't pick up any of the styles from theme.json automatically.

I'm curious if you see this method helping to improve that at all or if just reworking how we apply button styles a bit is an equally good solution.

Part of the problem your describing here is what we ended up needing to solve for QCHI on. The button block markup was used in the QCHI masthead, but we weren't specifically implementing the button block, so the styles never loaded if another button block wasn't on the page. This led us to implement a "global" flag for core blocks that we could set to force load the styles globally for that block (https://github.com/wpcomvip/qchi-lendnation/blob/develop/plugins/core/src/Blocks/Block_Base.php#L64-L88).

I think the other problem is that the markup for the button block has that extra wrapper, which makes it pretty difficult to use that styling elsewhere. Because the wrapping element gets the style class (.is-style-primary, etc) and the actual link element itself doesn't change, it makes it pretty hard to globally style buttons without duplicating code somewhere.

One last issue that comes to mind is that theme.json is extremely picky about what you're able to style within it. Although you can add custom CSS to theme.json, you can only do so for the blocks / elements they've allowed to be styled in there. For example, you can't just throw "input" as an object in the styles.elements array because theme.json just doesn't know what to do with it.

I think I would be open to styling the elements you mentioned separately elsewhere from the button block if we can define a global style for them, although, both the form submit button and the file download block would probably be specific to certain sites and not applicable to all sites. So I don't know if there's value yet in making an update like that, since we just don't have enough examples of using them.

@ChrisMKindred
Copy link
Contributor

I still don't completely understand the problem with using styles. I am sure it is ignorance on my side but bear with me while I try to clear it up...

Why is the primary style not the default for all buttons? If the primary were default, we would only need 2 styles, secondary and ghost. The end user adds a button to the page, it is the primary style by default. If they need to change it, they select the secondary or ghost style, and to change it back, they unselect the style. I am sure I am missing something either in our process or implementation that keeps this from being possible, I am just not sure what right now...

@GeoffDusome
Copy link
Contributor Author

I still don't completely understand the problem with using styles. I am sure it is ignorance on my side but bear with me while I try to clear it up...

Why is the primary style not the default for all buttons? If the primary were default, we would only need 2 styles, secondary and ghost. The end user adds a button to the page, it is the primary style by default. If they need to change it, they select the secondary or ghost style, and to change it back, they unselect the style. I am sure I am missing something either in our process or implementation that keeps this from being possible, I am just not sure what right now...

The main issue with block styles is that the "Default" block style, specifically for buttons (I'm sure it's the same for other blocks, but the button block is the only place we're seeing it a lot), doesn't get a class.

Implementing additional block styles makes the "Default" block style required. You are not able to remove it. Which goes back to one of my earlier comments where I stated that we could style the "Default" button style by chaining :not() selectors: :not(.is-style-secondary, .is-style-ghost) .wp-block-button__link, but that feels gross.

If we remove all button block styles, we're also able to remove the default, because there's nothing additional to show there, so the button is always "Default". Using block variations means we can always use one of the three classes we've defined for buttons (.is-style-primary etc) on those buttons, so they are more easily targeted for styling.

@dpellenwood
Copy link
Collaborator

Thanks for taking a stab at this @GeoffDusome. I have 2 hesitations with this approach:
First, the purpose of a block style (from the Block Editor Handbook) is to "...allow alternative styles to be applied to existing blocks." Whereas, the purpose of a block variation is to:

define multiple versions (variations) of a block. A block variation differs from the original block by a set of initial attributes or inner blocks. When you insert the block variation into the Editor, these attributes and/or inner blocks are applied.
Variations are an excellent way to create iterations of existing blocks without building entirely new blocks from scratch.

I'm concerned that while this is a nifty workaround to the actual issue we have, it creates a dependency on a block API which isn't intended to be used this way. I realize this is a bit of a philosophical debate versus a practical one, but it could have practical ramifications if core leans into the difference between these two APIs more in the future.

My second concern is that the UX in the editor for selecting/changing a variation versus a style is different enough that it could create issues for editors. Especially if other blocks (core or otherwise) use the Style API to adjust styles, but the Buttons block uses the variations API. It creates an inconsistent experience for an editor.

Continuing the thinking from my first concern, what if the variations UX changes even more significantly in a future release? It could be even less intuitive for editors to change a button's look & feel if the variations API becomes more about transforms and less about how a block looks.

Digging a bit of digging through the Gutenberg repo, I see many semi-related issues around default styles for core blocks. I wonder if it wouldn't be prudent to leave our (albeit, less than ideal) solution in place and let Core sort out the default style capabilities a bit?

@GeoffDusome
Copy link
Contributor Author

Closing - we've chosen to stick with Block Styles for this implementation.

@GeoffDusome GeoffDusome deleted the try/button-variations branch September 13, 2023 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants