[Experiment] Blocks: Add a new shared Button block

[Copons]

Alternative approach to #14920

See #14935

Changes proposed in this Pull Request:

In #14920 I've tried creating a Button component, complete with utility functions, ready to be used inside existing blocks, replacing our many re-implementations of the Core's Button block.

Watching some WPBlockTalk talks and talking with @apeatling made me rethink this approach, and try with InnerBlocks, which would have many UX improvements over the Button components.

Unfortunately, we still can't use the Core's Button as-is. Our use cases are different, and we need different features, and more flexibility.

Some features of this new jetpack/button block:

• Server-side rendering (default).
• Can be saved in post content (optional).
• Can output as a, input type="submit", button type"submit". (If saved in post content, it's forced as a because otherwise the WPCOM KSES would strip it).
• Can be extended with more attributes, data-attributes, etc.

Before committing to this approach, we should see if it actually works for all blocks currently using buttons.
Examples: #15377

Note for @Automattic/explorers

Most of this PR is copypasted from #14920.
Namely, the Button block files is pretty much === the Button component from the other PR, but with adjusted attribute names (buttonTextColor becomes textColor as it's clear that it refers to the button now).

The big differences are in how it's used by Revue.
This PR is slightly larger because changing from component to inner block makes the deprecation/migration a bit more verbose (especially in the PHP side).

Minor note: there's currently no fill/outline style variations. We can easily add them in a second moment if we want (and we do: Eventbrite uses them!)

Testing instructions:

• Create a new post.
• Add a Revue block, and insert a Revue username.
• Make sure the Button inside the Revue block works as expected.
• Save the post and view it.
• Make sure the button looks as expected.
• Try filling the Revue form and submitting it.
• Make sure the form works as expected.
• Deactivate Jetpack and reload the post on the front end.
• Make sure the form has been replaced with a simple link to the Revue profile.

Proposed changelog entry for your changes:

• N/A

[jetpackbot]

Thank you for the great PR description!

When this PR is ready for review, please apply the [Status] Needs Review label. If you are an a11n, please have someone from your team review the code if possible. The Jetpack team will also review this PR and merge it to be included in the next Jetpack release.

Scheduled Jetpack release: May 5, 2020.
Scheduled code freeze: April 28, 2020

Generated by 🚫 dangerJS against 61745ec

[Copons]

I've tried this with the Eventbrite block in #15377.
It seems to work well, but there are a few relatively minor issues that emerged during my tests:

• InnerBlocks doesn't work with BlockStylesSelector, probably because of an unreported infinite loop.
  To make it work, I had to render InnerBlocks behind a ! attributes.__isBlockPreview condition, which means that, in the style selector, the "button" style preview is empty.

• Typically, our embed blocks can either be an embedded iframe, or just a button.
  When a block contains only an InnerBlocks with Button, it's extremely hard to select the parent block: clicking around always end up selecting the inner Button, and I basically always had to use the block navigation tool.

• In some cases, the inner Button uses some of the parent block's attributes.
  In Eventbrite, for example, it needs both the url (to build the link href) and the eventId (for the widget ID).
  As far as I can see, we can pass variable values to the InnerBlocks template, but only on mount (aka: changes won't propagate to the inner blocks).
  In my tests this might not be a big deal: the Eventbrite block sets both url and eventId before transitioning from placeholder to "actual", and so InnerBlocks mounts with the correct values; and also when editing an existing post, InnerBlocks will mount with the existing values stored in the post content.
  I wonder if we might end up in a situation where we modify those values and InnerBlocks keeps using some old values, which would be a very hard to spot issue.
  Anybody knows if there's a way to keep InnerBlocks in sync with the parent attributes?

cc @mtias that might have some answers!

[Copons]

Ok! I think I've addressed 2 out of 3 issues!
cc @pablinos @apeatling

InnerBlocks breaks BlockStylesSelector

Turns out, @pablinos had the right hunch in #15377 (comment), and the issue had something to do with the block example.
Specifically, not the Button block's example, but the example of the parent block!

If the parent block contains InnerBlocks, we need to specify them in the example too.
Here's the fix in the Eventbrite example: a8fe2c4

Impossible to select the parent block when Button is the only block

This was because the Button block is rendered as display: block and without margins.
When Button is the only block, it completely fills the parent block's clickable area.

The workaround I'm proposing in bd15da3 simply sets it to display: inline-block.
This way, the button element is the only clickable area of the Button block, and clicking on all the remaining whitespace would select the parent block.

	Before	After
Click on Button
Click outside Button

This is not a perfect solution, but it definitely feels better.
We could add some vertical margin to make sure that there's always at least some clickable area for the parent block, but I don't have any strong opinions about it.

[apeatling]

Anybody knows if there's a way to keep InnerBlocks in sync with the parent attributes?

@Copons I implemented this in my contact form block updates in #15362

I select the parent block in the child block here:
48d144d#diff-161c61664862886e8b8ccd593d7ef3ccR128

Then check for differences between the child block attribute value, and the parent block attribute value:
48d144d#diff-63e0f976d4a767879d6304eacac73a3cR26

This works, but I'm sure there is a better way, it might give you some ideas though.

[apeatling]

Nitpick: Should this be capitalized? UseGradient

[Copons]

It shouldn't: useGradient is a hook, like useEffect.

[Copons]

Anybody knows if there's a way to keep InnerBlocks in sync with the parent attributes?

@Copons I implemented this in my contact form block updates in #15362

Thanks @apeatling, that's really interesting!

I'm playing around that example, and it's indeed of great value.
My idea right now is to add a passthroughAttributes attribute to the Button, where the parent block can map its own attributes keys into the Button attributes, then the Button would "apply" the map, check if the value is different, and update its attributes.

Example (based roughly on Eventbrite)

Eventbrite has an eventId attribute that needs to be passed through to the Button to build an element ID.
The Button has a uniqueId attribute to cover this case (which currently is not kept in sync with the parent).

// Parent block

<InnerBlocks template={ [ [
  'jetpack/button',
  {
    passthroughAttributes: {
      uniqueId: 'eventId', // { buttonAttribute: 'parentAttribute' }
    },
  }
] ] } />

// Button block

withSelect( ( select, { attributes, clientId } ) => {
  const { passthroughAttributes } = attributes;
  // { uniqueId: 'eventId' }

  const { getBlockAttributes, getBlockHierarchyRootClientId } = select( 'core/block-editor' );
  const parentAttributes = getBlockAttributes( getBlockHierarchyRootClientId( clientId ) );

  const mappedAttributes = mapValues( passthroughAttributes, key => parentAttributes[ key ] );
  // { uniqueId: 123456 } (actual parent's `eventId` value)

  // Discard all equals attributes
  const attributesToSync = pickBy( mappedAttributes, ( value, key ) => value !== attributes[ key ] );

  return { attributesToSync };
} )( ButtonEdit );


function ButtonEdit( { attributesToSync, setAttributes } ) {
  useEffect( () => {
    if ( ! isEmpty( attributesToSync ) ) {
      setAttributes( attributesToSync );
    }
  }, [ setAttributes, attributesToSync ] );
}

[Copons]

@Automattic/explorers I think this is already way too big to handle.
I'm proposing to move forward with this approach, which to me feels better and more Gutenberg-y than its component counterpart (#14920).

There are two clear outstanding tasks, but we should take care of them in follow ups:

• Keep the Button attributes in sync with its parent block (see [Experiment] Blocks: Add a new shared Button block #15370 (comment) for a proposed approach).
• Improve the parent block clickable area when the Button is floating (align left/right).

This is not currently urgent, but reviews are appreciated nonetheless! 🙇‍♂️
As a note: most of the code here is copypasted from the component approach, so if you gave that a look, this is pretty much identical. 🙂

[scruffian]

This is looking good. The only thing I noticed was that the button display in the front end and the editor looks off, but this isn't just a problem with this block, I see it with buttons in all our blocks:

This might be an issue with my Jetpack/Gutenberg setup.

Edit: I just updated my Gutenberg install and this fixed itself!

[scruffian]

I think this looks good, lets merge and iterate.

[jeherve]

When I enter a custom text for the button content, it does not appear on the frontend.

---

For some reason, this makes me lose all blocks in the editor in WordPress 5.3.2. Anyone else can reproduce?

[jeherve]

Should it be in formatting?

[Copons]

Since this block doesn't show up in the inserter, this doesn't matter much.
Though, I've updated this to layout, which is the Core Button category.

[jeherve]

TIL. Fancy.

[Copons]

@jeherve

When I enter a custom text for the button content, it does not appear on the frontend.

Fixed, it was an incorrect strpos check that I was sure I had fixed days ago. Likely forgot to commit it and dropped the change. 🤷

For some reason, this makes me lose all blocks in the editor in WordPress 5.3.2. Anyone else can reproduce?

I'm testing with yarn docker:wp core update --version=5.3.2 --force and I can't see any noticeable issues. 🤔
Do you have any additional repro steps I could try?

Also, I've pushed a fix for the Revue example, adding the inner blocks definition in there as well.
It prevented the block preview to show, breaking the entire inserter. Could be related?

[apeatling]

@Copons Sorry I wasn't meaning to imply that you should do it. 😀

I wanted to make sure that's an option for when this is merged that I can implement. Totally agree no more should go into this PR, I can always open a followup PR after. 👍

[scruffian]

This only seems to show when the colors are hovered. Once I move the mouse away it disappears...

[scruffian]

This looks good to me. One small issue that I think we can fix in a follow up PR...

[matticbot]

Caution: This PR has changes that must be merged to WordPress.com
Hello Copons! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D42493-code before merging this PR. Thank you!
This revision will be updated with each commit to this PR

[jeherve]

This seems to work well, I'd suggest merging this as is.

I found one issue with the button though:

This can be fixed in a follow-up PR though.

[jeherve]

Do you think you could add a comment to explain what's going on here? It took me a minute to understand where that class was coming from.

[Copons]

r206525-wpcom

[apeatling]

@Copons I've found a bug I think, the border radius setting does not seem to stick when the editor is reloaded. It saves the attr value so it's rendered on the front end correctly but not back in the editor. Seems to be the case in the Revue block, and using it in #15362.

[apeatling]

Found a fix for this in #15597

[kraftbj]

r206618-wpcom (removing a file missed by r206525-wpcom)