SubmitButton: Make it use the theme palette #14599

pablinos · 2020-02-07T20:18:14Z

The SubmitButton component was setting the customButtonTextColor
(or the customButtonBackgroundColor) attribute whenever a colour was
set. This would override any colour classes set to use the theme
palette, like has-primary-color. As a result the colours wouldn't
update as the theme was changed.

This also meant that a few components were only rendering the button
using the custom colour attributes, and so would suffer from the same
problem.

Additionally, the Calendly attribute validation was not allowing colour
names like primary. For now, I've removed the validation as everything
is properly escaped, but in future, we should probably create a better
regex to check for hex values or hyphenated lowercase words.

Changes proposed in this Pull Request:

This change updates the SubmitButton to respect the theme palette and set the submitButtonClasses attribute on the component where it is being used. As a result, I have also had to update various places where it has been used, so the classes are used when the button is rendered.

Is this a new feature or does it add/remove features to an existing part of Jetpack?

This is a minor bug fix although the validation change is critical for Calendly, so if we're not happy with this change as a whole, I'll separate that into another PR.

Testing instructions:

On a test site create a post with any (or all) of the Subscription, Calendly, Recurring Payments, and MailChimp blocks.
Set their colours using colours from the theme palette.
Preview the post and check that the correct colours are rendered.
Change the theme, and check that the colours change accordingly
Set a custom colour.
Check that this renders correctly.
Change the theme again.
Check that the same custom colour is used.

Proposed changelog entry for your changes:

SubmitButton now uses the theme's colour palette where possible

(The problem with this is that already saved blocks will have custom colour attributes, and won't benefit from this change)

matticbot · 2020-02-07T20:18:23Z

Caution: This PR has changes that must be merged to WordPress.com
Hello pablinos! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D38628-code before merging this PR. Thank you!

jetpackbot · 2020-02-07T20:20:08Z

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: March 3, 2020.
Scheduled code freeze: February 25, 2020

Generated by 🚫 dangerJS against e6b2bf2

scruffian · 2020-02-10T17:05:21Z

This works as described, but I have a few reservations about the way it works. I tested with the themes Twenty Nineteen and Twenty Sixteen, and they don't use terms like primary and secondary, they mainly use colour codes names, which don't match when you switch themes. I wonder how a user would feel about setting the colour of a button only for that to change when they changed themes...

...thinking...

Having said that, this is the way the core Button block works, so it's good to match that. I say 🚢

extensions/blocks/recurring-payments/edit.jsx

matticbot · 2020-02-10T17:28:07Z

pablinos, Your synced wpcom patch D38628-code has been updated.

scruffian · 2020-02-11T18:07:15Z

I noticed a few display issues with the recurring payments block, and pushed a fix

pablinos · 2020-02-12T07:22:48Z

I tested with the themes Twenty Nineteen and Twenty Sixteen, and they don't use terms like primary and secondary, they mainly use colour codes names, which don't match when you switch themes. I wonder how a user would feel about setting the colour of a button only for that to change when they changed themes...

Yeah, it's a tricky trade-off. There are some consistency in colour names between Twenty Nineteen and Twenty Twenty, and some of the others use names like light_gray which are reused. I personally think if I've selected a colour from the theme palette and then change themes, I would be fine with that colour changing or falling back to the default. If the user picks a custom colour then that will be retained.

It would be good if we encouraged theme developers to standardise a core set of 5 colour names.

pablinos · 2020-02-12T07:24:59Z

modules/memberships/class-jetpack-memberships.php

@@ -270,12 +251,13 @@ public function render_button( $attrs ) {
 		$button_styles = implode( ';', $button_styles );
 		add_thickbox();
 		return sprintf(
-			'<button data-blog-id="%d" data-powered-text="%s" data-plan-id="%d" data-lang="%s" class="%s" style="%s">%s</button>',
+			'<div class="wp-block-button %s"><a role="button" data-blog-id="%d" data-powered-text="%s" data-plan-id="%d" data-lang="%s" class="wp-block-button__link %s" style="%s">%s</a></div>',


Yes, I wasn't sure whether we should do this or not. I didn't know if there was something relying on this being rendered as a button not wrapped in a div. I suppose the answer is that it shouldn't be!

scruffian · 2020-02-12T11:37:26Z

extensions/blocks/calendly/edit.js

-			setAttributes={ setAttributes }
-		/>
-	);
+	const submitButtonPreview = <SubmitButton { ...props } />;


It might be better to just pass the props that are needed? (https://codeburst.io/react-anti-pattern-jsx-spread-attributes-59d1dd53677f)

What do you think?

Shame on you linking to Medium 😉

Yes, it did strike me as a bit lazy, copied it from one of the other uses.

pablinos · 2020-02-12T15:55:33Z

I'm wondering if we should solve this by using the core button component in some way and phase out the use of SubmitButton. I'm not sure how long it will take for us to do that though, so we may want to tidy this up an merge it in the meantime

scruffian · 2020-02-12T17:02:41Z

Yeah I think we should stick with this for now.

I wonder if the best way to implement the core button would be to replace pieces of this with things from the core button...

jeherve

This is going to need a rebase I'm afraid.

The `SubmitButton` component was setting the `customButtonTextColor` (or the `customButtonBackgroundColor`) attribute whenever a colour was set. This would override any colour classes set to use the theme palette, like `has-primary-color`. As a result the colours wouldn't update as the theme was changed. This also meant that a few components were only rendering the button using the custom colour attributes, and so would suffer from the same problem. Additionally, the Calendly attribute validation was not allowing colour names like `primary`. For now I've removed the validation as everything is properly escaped, but in future we should probably create a better regex to check for hex values or hyphenated lowercase words. refactor recurring payments class names to make the code more readable use links for the recurring payments button, so it matches the other uses of this block Add positional markers to the arguments to sprintf Add check for submitButtonClasses being set Remove className attribute from SubmitButton The classes being passed into SubmitButton appear to be redundant. Thes removes them and explicitly passes just the props needed. Make attribute check clearer Format the attribute definition consistently Explicitly pass the required props to SubmitButton

matticbot · 2020-02-24T16:45:52Z

pablinos, Your synced wpcom patch D38628-code has been updated.

scruffian · 2020-02-24T16:46:08Z

Rebase ✅

Copons · 2020-02-24T17:04:06Z

This works fine for me.

I've just got one question: why are we creating all those submitButtonClasses attributes, when we never really use it? 🤔

pablinos · 2020-02-24T17:46:15Z

I've just got one question: why are we creating all those submitButtonClasses attributes, when we never really use it? 🤔

The submitButtonClasses attribute gets used in the various PHP functions that render the buttons. Eventually, we should move away from this and save the block in the editor, as we're doing with Eventbrite, but my reworking of the Calendly block (#14630) has thrown up some complications with doing that.

There are some other attributes, which might not be used. It would probably be good to clean that up if we can. Sounds like another PR to me though!

scruffian · 2020-02-24T18:01:05Z

Sounds like another PR to me though!

Definitely. We should get this merged soon as it keep going stale!

Copons · 2020-02-24T18:12:54Z

The submitButtonClasses attribute gets used in the various PHP functions that render the buttons.

Oooh, never mind, I've found now where we set it:

jetpack/extensions/shared/submit-button.js

Line 63 in 47bd913

this.props.setAttributes( { submitButtonClasses: buttonClasses } );

Somehow I totally missed that line before, and was sure we never set it anywhere 😅

jeherve

This seems to work well. 👍

* 8.3 release: changelog * Changelog: add #14516 * Changelog: add #14574 * Bring in changes from 8.2.1 and 8.2.2 * Update stable version * Bring in 8.2.3 changes * Changelog: add #14714 * Changelog: add #14639 * Changelog: add #14678 * Changelog: add #14673 * Changelog: add #14687 * Changelog: add #14704 * Changelog: add #14702 * Changelog: add #14541 * Changelog: add #14657 * Changelog: add #14622 * Changelog: add #14582 * Changelog: add #14638 * Changelog: add #14633 * Changelog: add #14571 * Changelog: add #14592 * Changelog: add #14539 * Changelog: add #14514 * Changelog: add #14643 * Changelog: add #14494 * Changelog: add #13739 * Changelog: add #14707 * Changelog: add #14736 * Changelog: add #14706 * Changelog: add #14730 * Changelog: add #14685 * Changelog: add #14727 * Changelog: add #14711 * Changelog: add #14742 * Changelog: add #14746 * Changelog: add #14725 * Changelog: add #13999 * Changelog: add #14740 * Changelog: add #14759 * Changelog: add #14703 * Changelog: add #14753 * Changelog: add #14754 * Changelog: add #14645 * Cahngelog: add #14599

scruffian · 2020-02-26T13:19:53Z

r203415-wpcom

pablinos requested a review from a team February 7, 2020 20:18

matticbot added the Touches WP.com Files label Feb 7, 2020

pablinos self-assigned this Feb 7, 2020

pablinos added [Block] Calendly [Block] Mailchimp [Block] Subscriptions [Block] Payment Button aka Recurring Payments labels Feb 7, 2020

pablinos added the [Status] Needs Review This PR is ready for review. label Feb 7, 2020

pablinos requested review from scruffian and jeherve February 7, 2020 20:28

jeherve added this to the 8.2 milestone Feb 10, 2020

jeherve added the [Status] Needs Cherry-Pick label Feb 10, 2020

scruffian reviewed Feb 10, 2020

View reviewed changes

extensions/blocks/recurring-payments/edit.jsx Outdated Show resolved Hide resolved

scruffian mentioned this pull request Feb 10, 2020

Calendly: Allow theme color names to be saved in the submit button #14625

Merged

jeherve modified the milestones: 8.2, 8.3 Feb 10, 2020

jeherve removed the [Status] Needs Cherry-Pick label Feb 10, 2020

creativecoder requested a review from a team February 11, 2020 17:47

pablinos commented Feb 12, 2020

View reviewed changes

scruffian reviewed Feb 12, 2020

View reviewed changes

dereksmart added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Feb 12, 2020

jeherve added the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Feb 24, 2020

jeherve requested changes Feb 24, 2020

View reviewed changes

scruffian force-pushed the fix/submit-button-colours branch from 6bc57e7 to e6b2bf2 Compare February 24, 2020 16:45

scruffian added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels Feb 24, 2020

Copons approved these changes Feb 24, 2020

View reviewed changes

jeherve approved these changes Feb 24, 2020

View reviewed changes

jeherve added [Status] Needs Testing We need to add this change to the testing call for this month's release [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Feb 24, 2020

jeherve merged commit 5ddf14c into master Feb 24, 2020

jeherve deleted the fix/submit-button-colours branch February 24, 2020 23:14

matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Feb 24, 2020

jeherve added [Status] Has Changelog and removed [Status] Needs Changelog [Status] Needs Testing We need to add this change to the testing call for this month's release labels Feb 25, 2020

jeherve added a commit that referenced this pull request Feb 25, 2020

Cahngelog: add #14599

f58c5f7

scruffian mentioned this pull request Feb 26, 2020

Recurring payments: Look for the data attributes on the right element #14809

Merged

pablinos mentioned this pull request Mar 6, 2020

Contact Form Block: Fix Missing Button Color Attributes #14898

Merged

SubmitButton: Make it use the theme palette #14599

SubmitButton: Make it use the theme palette #14599

Uh oh!

Conversation

pablinos commented Feb 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this Pull Request:

Is this a new feature or does it add/remove features to an existing part of Jetpack?

Testing instructions:

Proposed changelog entry for your changes:

Uh oh!

matticbot commented Feb 7, 2020

Uh oh!

jetpackbot commented Feb 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scruffian commented Feb 10, 2020

Uh oh!

Uh oh!

matticbot commented Feb 10, 2020

Uh oh!

scruffian commented Feb 11, 2020

Uh oh!

pablinos commented Feb 12, 2020

Uh oh!

pablinos Feb 12, 2020

Choose a reason for hiding this comment

Uh oh!

scruffian Feb 12, 2020

Choose a reason for hiding this comment

Uh oh!

pablinos Feb 12, 2020

Choose a reason for hiding this comment

Uh oh!

pablinos commented Feb 12, 2020

Uh oh!

scruffian commented Feb 12, 2020

Uh oh!

jeherve left a comment

Choose a reason for hiding this comment

Uh oh!

matticbot commented Feb 24, 2020

Uh oh!

scruffian commented Feb 24, 2020

Uh oh!

Copons commented Feb 24, 2020

Uh oh!

pablinos commented Feb 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scruffian commented Feb 24, 2020

Uh oh!

Copons commented Feb 24, 2020

Uh oh!

jeherve left a comment

Choose a reason for hiding this comment

Uh oh!

scruffian commented Feb 26, 2020

Uh oh!

Uh oh!

pablinos commented Feb 7, 2020 •

edited

Loading

jetpackbot commented Feb 7, 2020 •

edited

Loading

pablinos commented Feb 24, 2020 •

edited

Loading