SubmitButton: Make it use the theme palette#14599
Conversation
|
Caution: This PR has changes that must be merged to WordPress.com |
|
Thank you for the great PR description! When this PR is ready for review, please apply the Scheduled Jetpack release: March 3, 2020. |
|
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 ...thinking... Having said that, this is the way the core Button block works, so it's good to match that. I say 🚢 |
|
pablinos, Your synced wpcom patch D38628-code has been updated. |
|
I noticed a few display issues with the recurring payments block, and pushed a fix |
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 It would be good if we encouraged theme developers to standardise a core set of 5 colour names. |
There was a problem hiding this comment.
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!
extensions/blocks/calendly/edit.js
Outdated
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Shame on you linking to Medium 😉
Yes, it did strike me as a bit lazy, copied it from one of the other uses.
|
I'm wondering if we should solve this by using the core button component in some way and phase out the use of |
|
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
left a comment
There was a problem hiding this comment.
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
6bc57e7 to
e6b2bf2
Compare
|
pablinos, Your synced wpcom patch D38628-code has been updated. |
|
Rebase ✅ |
|
This works fine for me. I've just got one question: why are we creating all those |
The 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! |
Definitely. We should get this merged soon as it keep going stale! |
Oooh, never mind, I've found now where we set it: Somehow I totally missed that line before, and was sure we never set it anywhere 😅 |
* 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
|
r203415-wpcom |
The
SubmitButtoncomponent was setting thecustomButtonTextColor(or the
customButtonBackgroundColor) attribute whenever a colour wasset. This would override any colour classes set to use the theme
palette, like
has-primary-color. As a result the colours wouldn'tupdate 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 everythingis 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
submitButtonClassesattribute 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?
Testing instructions:
Proposed changelog entry for your changes:
(The problem with this is that already saved blocks will have custom colour attributes, and won't benefit from this change)