Jetpack: Update the SubmitButton component and refactor all the blocks to use it.

[pablinos]

Background

During recent development of various blocks, there has been a need to have a Button component, that worked in the same way as the core/button Gutenberg block but with minor adjustments to how it was saved or rendered on the server.

There is a SubmitButton component that is used by a number of blocks as an edit component e.g. Calendly Subscriptions MailChimp and others. This means that submitButton attributes are set on the block, which are then used in the save function or when the block is rendered on the server.

As the SubmitButton component is out of sync with its core equivalent, and it's not well documented, the Eventbrite block has implemented its own version of it and another version even closer to the core one is being proposed for the Revue block.

A couple of options

@mtias has suggested (p1581450683149300-slack-explorers) a solution to this that would involve using the new block variations API. From some initial investigations, this could be possible, but the API won't be available for use by Jetpack for some time, and I'm not convinced that it would allow us to create the same UI. Having said that, it's something to consider for the future.

We did also consider making the core/button block's edit and save functions public, so that they could be reused in the Jetpack blocks and elsewhere. This could cause future problems as they get used in unforeseen scenarios that would then need to be supported. It would be possible to get at these functions with the blocks.registerBlockType filter, but that's somewhat of a hack.

The way core Gutenberg development is going, various features are being abstracted into hooks, so it may also be possible to compose our own version of the button component in a simple manner in the future.

A plan of what to do next

While we wait for the core Gutenberg APIs to become usable in the minimum supported Jetpack WordPress version, I propose that we:

• Update the SubmitButton component to include some of the colour and gradient settings that are now in core Gutenberg.
• Add a save function to handle the majority of cases. We now have a pattern of saving the button HTML in the post_content and updating it dynamically with an id attribute on the server. We should create helper functions to handle this.
• Refactor any uses of SubmitButton to conform to the pattern we're using
• Check that there aren't other button components created as part of other blocks that could be changed to use our SubmitButton implementation
• Potentially rename it to something more generic, JetpackButton, BlockButton, something else?

Then we monitor development in Gutenberg and hopefully deprecate it altoghether in the future.

[creativecoder]

@pablinos @scruffian and I discussed this further, and agreed on the following plan

1. Propose updating the block-library/button component so it can be used in place of SubmitButton (and other custom solutions) in Jetpack blocks (@pablinos will create an issue in Gutenberg)
2. Update all the blocks (Calendly, Eventbrite, Revue, Mailchimp, Subscriptions, Contact Form, and Recurring Payments) with the Gutenberg solution, when it's ready
3. In the meantime, settle on a solution for new blocks that doesn't further fragment our code

@Copons For #3, we have a new implementation of a button in the Revue block. Do you think we should re-use that going forward, for new blocks?

[Copons]

My opinion is that we should follow strictly whatever Core does as much as possible in order to save us headaches in the long run.
The Revue implementation is basically a mix of the Core Button (for the edit side) and Navigation blocks (for the PHP side, as Button doesn't render server-side), updated at the day I wrote it, with 1 major difference (checks for gradient existence).

Making the SubmitButton as close as possible to Core means that porting Core new features back to Jetpack will be considerably easier, and with less chances of regressions.

But, while I'd be tempted to say "Let's update SubmitButton to the Revue implementation", I think things are a bit more complicated.

Attributes

Core Button is designed as a standalone element, and so its attributes are unequivocal.
text, textColor, gradient, borderRadius are unequivocally the _button_'s text, color, etc.

This doesn't necessarily apply to our cases, when the SubmitButton is only a part of a block consisting of more elements.
Going full-Core on Revue had the side effect of making its attributes totally confusing (my bad here, I've realized it too late).
E.g. why does textColor only affect the button's color instead of the whole form?

The current SubmitButton implementation already solve this by increasing the attributes specificity: submitButtonText, textButtonColor, etc.
It could be argued that names could be more consistent, but their meaning is immediately clear.

Proposal

Let's go the SubmitButton way.
Maybe give the attribute names a consistent "order": buttonText (or submitButton- to be even more specific), buttonTextColor, buttonBackgroundColor, buttonBorderRadius, etc.

Post Content and Server-side Rendering

Buttons can be used in many different ways:

• Core Button is in fact a simple link styled as a button, and it is safely saved in the post content.
• In most of our blocks the SubmitButton is a link with an onClick, which would be stripped by the KSES, and so it can't be saved in the post content, but server-side rendered.
• Revue uses it to submit a form, so it can be either an input or a button of type="submit". They both are stripped by the KSES, and so they need to be server-side rendered as well.

This is probably the biggest difference between us and Core.
In some cases (e.g. Eventbrite) we can save it as a simple link in the post content, and then, in PHP (or in the view too) add a click event listener on the element via its ID.
In others (e.g. Revue) we actually need to have a plain and simple HTML element that's not allowed by KSES.

Proposal

We need options!

1. Save the button as a simple link in the post content.
2. Save the button as a <a> with an ID in the post content, and then add the onClick in the view.js (I'd personally rather do it in JS than add an inline script in PHP, but I might be missing some context).
3. Don't save anything in the post content, and render it in PHP.

I guess that we'll have 2 JS helper functions (for save and for view) for 1 and 2.
I'd say that, for now, we don't need anything specific for the Revue SSR, as it's the only PHP-only case.

Button Styles

I've already voiced my disagreement with using wp_add_inline_style to add custom styles to the block's server-side rendering, and I still think I'm right. 😄

Typically, colors can be:

• Named, defined and styled by the theme as CSS classes. E.g. primary, defined by the theme, and then used as the .has-text-color.has-primary-text-color class.
• Custom, picked by the user with the color picker, and saved as hex code. E.g. #123456

We can all agree that if the color is named, we just need to add the respective class to the element and call it a day.
E.g.

<a class="has-text-color has-primary-text-color">Submit</a>

But, when it comes to custom colors, I think wp_add_inline_style complicates and obfuscates all the things.
The intent of a custom color is clearly to apply a custom color to a single element, which seems to be the perfect use case for inline styles.
On the other hand, wp_add_inline_style outputs a new <style> block in the page, that needs to target an element ID.
E.g.

<!-- Inline style -->
<a style="color: #123456;">Submit</a>

<!-- `wp_add_inline_style` -->
<a id="foobar-123">Submit</a>
<style id="jetpack-foobar-external-css">
  #foobar-123 { color: #123456; }
</style>

In both cases, overriding the custom color would probably need an !important, but I don't really see this happening, considering the user has picked this color by their own.

On top of this, when the block is saved in JS, we already save custom colors as inline styles, so why not be consistent? 🙂

Proposal

When the block is saved in the post content, let's do it like Core and us do.
When the block is rendered in PHP, let's do it like... Core and Revue do.

[jeherve]

@Copons 👍 You've convinced me for the use of inline styles. :)

[Copons]

👍 You've convinced me for the use of inline styles. :)

[pablinos]

You'd convinced me too @copons, which is why I changed it in the SubmitButtonSave function in this PR

I somehow missed your excellent summary of the situation here. I'll link to it from the tracking issue #14935. We also have multiple PRs that have elements of the solution in them, so I think we should add anything missing from #14630 and this one to the new #14920 and #14931, and abandon the other two

As a side note, having a ButtonSave function for use when we want a link is useful, and we could maybe create some helper functions for the PHP side too.

I agree that we need to make the naming more consistent, and we should move away from SubmitButton because it's not always submitting something, but you've already done that in #14920, which is great!

[Copons]

I agree that we need to make the naming more consistent, and we should move away from SubmitButton because it's not always submitting something, but you've already done that in #14920, which is great!

@pablinos to be honest, I've chosen the simple "Button" because it was the shortest option, especially for the attributes buttonBackgroundColor is easier to write than theNewSharedSubmitButtonComponentBackgroundColor 😄

[Copons]

I've tried using these changes with my Button experiment: https://github.com/Automattic/jetpack/pull/14931/commits/787ed48af2aa2bca8a96fee3edf1eb8805748a51

I haven't done anything for the deprecation/migration path, but with _new_ blocks it seems to be working fine!
It didn't need any changes to Button, except for updating data-id-attr="placeholder", and an unrelated issue. 🙂

[creativecoder]

Closing this issue. We'll open separate issues for the remaining blocks that need to be updated to use the new button component: Eventbrite, Calendly, Mailchimp, Recurring Payments, Subscriptions