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

Buttons Block: Show inserter if button have variations (#53498) #53783

Merged
merged 3 commits into from Sep 11, 2023

Conversation

petitphp
Copy link
Contributor

@petitphp petitphp commented Aug 17, 2023

What?

This PR update the Buttons block to show the inserter if the button have variations. Fix #53498

Why?

Currently, when adding a button's variation with the default scope, there is no way to use that variation due to the directInsert option in the innerBlocksProps which bypass the inserter. The only way is to add the transform scope.

How?

This PR change the value of the directInsert option if button variations exists.

Testing Instructions

Snippet to register a button variation :

wp.blocks.registerBlockVariation('core/button', {
	name: 'custom-button',
	title: 'My custom button',
	scope: ['transform', 'inserter', 'block'],
	attributes: {className: 'my-custom-button'},
});

Instructions

  1. Edit a post,
  2. Add a Buttons block,
  3. Add a button, the button should be added directly without showing the inserter,
  4. Using the browser console, register a button variation,
  5. Add a button, the inserter should appear and let you select between the default button and the variation.

Screenshots or screencast

buttons_block_inserter

@stokesman stokesman added [Type] Enhancement A suggestion for improvement. [Block] Buttons Affects the Buttons Block labels Aug 17, 2023
Copy link
Contributor

@stokesman stokesman left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I tested it out and found the direct insert works as before when no variations are registered and when variations are registered the menu is available. The code looks fine to me as well. It might be a smidge better if instead of adding another useSelect the existing one were reused. You could change that if you'd like or otherwise I'll get to it in #53786 (I'll keep that drafted in deference to getting this PR dealt with first).

Google.Chrome.-.Edit.Post.My.WordPress.Website.WordPress.2023-08-17.at.3.10.46.PM.mp4

@Mamaduka
Copy link
Member

A more general solution would be to solve this in the Inserter component, so every block using the exact option doesn't have to handle it separately.

I think shipping this solution is okay, but let's include a comment that we want Inserter to handle this logic eventually.

P.S. Meanwhile, the Inserter component could use some refactoring 😄

@petitphp
Copy link
Contributor Author

petitphp commented Aug 18, 2023

@stokesman Thanks for the review. I've applied your suggestion and merge the new useSelect with the existing one.

@Mamaduka I've added a comment. I don't know enough Gutenberg to see what should be refactored in the Inserter but happy to help work on it !

@Mamaduka
Copy link
Member

Thanks for the follow-up, @petitphp!

I've Inserter on my todo list, and I hope I'll find time before WP 6.4.

P.S. Failing test isn't related to this PR, but we'll have to wait until the project's flaky Playwright test is resolved.

@stokesman
Copy link
Contributor

Thanks for rebasing @petitphp. I'll go ahead a pull the trigger here since all tests are passing.

@stokesman stokesman merged commit ef5bd83 into WordPress:trunk Sep 11, 2023
48 checks passed
@github-actions github-actions bot added this to the Gutenberg 16.7 milestone Sep 11, 2023
@petitphp petitphp deleted the issue/53498 branch September 11, 2023 19:17
}, [] );

const innerBlocksProps = useInnerBlocksProps( blockProps, {
allowedBlocks: ALLOWED_BLOCKS,
defaultBlock: DEFAULT_BLOCK,
directInsert: true,
// This check should be handled by the `Inserter` internally to be consistent across all blocks that use it.
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this considered a follow-up? What prevented us from actually implementing this solution?

Copy link
Contributor

Choose a reason for hiding this comment

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

I understood it to be for a follow-up.

What prevented us from actually implementing this solution?

I suppose nothing besides someone putting in the effort. I've taken a closer look and the logic is actually already there. It seems the Buttons block was working as in this PR before #37905. In that PR directInsert: true was added and which forces it even when there are variations. I think I found a way to reconcile this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Buttons Affects the Buttons Block Needs User Documentation Needs new user documentation [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

core/button variations doesn't appear in core/buttons inserter
5 participants