Skip to content

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

Merged
stokesman merged 3 commits intoWordPress:trunkfrom
petitphp:issue/53498
Sep 11, 2023
Merged

Buttons Block: Show inserter if button have variations (#53498)#53783
stokesman merged 3 commits intoWordPress:trunkfrom
petitphp:issue/53498

Conversation

@petitphp
Copy link
Copy Markdown
Contributor

@petitphp petitphp commented Aug 17, 2023 •
edited
Loading

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

@petitphp petitphp requested a review from ajitbohra as a code owner August 17, 2023 20:29
@stokesman stokesman added [Type] Enhancement A suggestion for improvement. [Block] Buttons Affects the Buttons Block labels Aug 17, 2023
stokesman
stokesman approved these changes Aug 17, 2023
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member

Mamaduka commented Aug 18, 2023

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
Copy Markdown
Contributor Author

petitphp commented Aug 18, 2023 •
edited
Loading

@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
Copy Markdown
Member

Mamaduka commented Aug 18, 2023

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
Copy Markdown
Contributor

stokesman commented Sep 11, 2023

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
@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
youknowriad
youknowriad reviewed Sep 11, 2023
View reviewed changes
packages/block-library/src/buttons/edit.js
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
Copy Markdown
Contributor

@youknowriad youknowriad Sep 11, 2023

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
Copy Markdown
Contributor

@stokesman stokesman Sep 12, 2023

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.

@stokesman stokesman mentioned this pull request Sep 13, 2023
Draft
@femkreations femkreations added the Needs User Documentation Needs new user documentation label Sep 30, 2023
@femkreations femkreations mentioned this pull request Oct 19, 2023
Open
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@youknowriad youknowriad youknowriad left review comments

@stokesman stokesman stokesman approved these changes

@ajitbohra ajitbohra Awaiting requested review from ajitbohra ajitbohra is a code owner

Assignees

No one assigned

Labels

[Block] Buttons Affects the Buttons Block Needs User Documentation Needs new user documentation [Type] Enhancement A suggestion for improvement.

Projects

None yet

Milestone

Gutenberg 16.7

Development

Successfully merging this pull request may close these issues.

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

5 participants

@petitphp @Mamaduka @stokesman @youknowriad @femkreations