Subscribe block: Add sync between button background color and border color #22949

ivan-ottinger · 2022-02-17T15:25:50Z

The proposed changes provide an automatic border color change when the user changes the button background color.

Screen recording displaying the proposed changes in action:

Screen.Capture.on.2022-02-18.at.16-23-18.mp4

Changes proposed in this Pull Request:

The PR introduces new useEffect() that automatically sets the borderColor to the currently set buttonBackgroundColor.

The borderColor is set automatically if and only if the borderColor holds the same color as the buttonBackgroundColor (including a case when the colors haven't been set yet; for instance when the block is added to the page). The condition also makes sure that if the user already set a custom color for their border manually, it won't get overridden.

Jetpack product discussion

p1645093761706199/1645006316.158349-slack-C02TCEHP3HA

Does this pull request change what data or activity we track or use?

No, it does not.

Testing instructions:

Add the Subscribe block to a post or a page
Adjust its Button Background Color and Border Color settings
If both colors are the same, changing the Button Background Color should automatically change the Border Color - as can be seen in the screen recording above

The commit introduces `useEffect()` that automatically sets the `borderColor` to the currently set `buttonBackgroundColor`. The `borderColor` is set only if the `borderColor` hasn't been set before.

matticbot · 2022-02-17T15:25:56Z

Caution: This PR has changes that must be merged to WordPress.com
Hello ivan-ottinger! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D75170-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

github-actions · 2022-02-17T15:27:21Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
✅ All commits were linted before commit.
✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.

Jetpack plugin:

Next scheduled release: March 1, 2022.
Scheduled code freeze: February 22, 2022.

The new condition makes sure the border color is changed if and only if: - both colors (border and button background color) are the same currently, and: - the new color we are setting the button background color to isn't the same as the current border color

yansern · 2022-02-22T07:21:46Z

In general, the code works and the automation is nice, but it raises some questions:

User sets background color and the border color changes. Could this be seen as unexpected behavior by the user?
https://user-images.githubusercontent.com/1287077/155081308-111b2118-1772-42e8-ac3d-29aaf1d933b0.mp4
After that, the user decides that the background color should be different from the border color, but the user could not do that, because it changes the border color.
https://user-images.githubusercontent.com/1287077/155081591-4d28914a-1013-41e6-91e3-903edf4cd6ba.mp4

yansern · 2022-02-22T07:24:08Z

Just another test on setting custom border color & custom background color. This is OK.
https://user-images.githubusercontent.com/1287077/155082628-651d9413-c05f-49fc-8873-099a0d9f0369.mp4

ivan-ottinger · 2022-02-22T08:45:55Z

After that, the user decides that the background color should be different from the border color, but the user could not do that, because it changes the border color.
https://user-images.githubusercontent.com/1287077/155081591-4d28914a-1013-41e6-91e3-903edf4cd6ba.mp4

Just another test on setting custom border color & custom background color. This is OK.
https://user-images.githubusercontent.com/1287077/155082628-651d9413-c05f-49fc-8873-099a0d9f0369.mp4

Yes, that's right. The idea is to automate the border color change only if the colors are the same. If the user changes the border color to some other color than the current background color, the color sync "is disabled" and additional changes to the background color won't affect the set border color.

This way, the user won't lose their custom border color.

At the same time there's the case you shared in your screen recording: https://user-images.githubusercontent.com/1287077/155081591-4d28914a-1013-41e6-91e3-903edf4cd6ba.mp4. Thank you for catching that, Yan!

When both colors are the same and the user wants to keep the border color and change only the background color, they would need to remember that border color - as if they switch the backgroud color to something else, the border color will be changed as well (as both colors were the same and color sync is "enabled").

Here's the conversation where Matiás shared the intended behavior: p1645093761706199/1645006316.158349-slack-C02TCEHP3HA.

The question is now what is better from the user's perspective. 🤔

yansern

Although I have a small usability concern (over the steps needed to unsync background color with the border color to change just the background colour), this PR does what it says, which is:

Setting background color automatically sets border color:

when the border color is unset OR
when the border color is the same color as the background color

I'll approve this, and will leave this for the 2nd round of review.

…-background-border-color-sync

github-actions · 2022-02-22T14:18:28Z

Great news! One last step: head over to your WordPress.com diff, D75170-code, and commit it.
Once you've done so, come back to this PR and add a comment with your changeset ID.

Thank you!

ivan-ottinger · 2022-02-23T09:44:47Z

Successfully deployed to WPCOM. Changeset: r240597-wpcom.

mtias · 2022-02-23T10:50:17Z

Thanks for working on this! We can always revise the logic but I think it covers the most common expectation of wanting background to also affect the border color.

Maybe in the future we can pair this with whether compact or split is being used, though that could add another layer of hidden UX.

Add sync between button background color and border color

907aaf1

The commit introduces `useEffect()` that automatically sets the `borderColor` to the currently set `buttonBackgroundColor`. The `borderColor` is set only if the `borderColor` hasn't been set before.

matticbot added the Touches WP.com Files label Feb 17, 2022

ivan-ottinger self-assigned this Feb 17, 2022

ivan-ottinger added the [Status] In Progress label Feb 17, 2022

github-actions bot added [Block] Subscribe [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ labels Feb 17, 2022

ivan-ottinger marked this pull request as ready for review February 18, 2022 15:45

oskosk added this to the jetpack/10.7 milestone Feb 22, 2022

ivan-ottinger added the [Status] Needs Team Review Obsolete. Use Needs Review instead. label Feb 22, 2022

ivan-ottinger requested a review from a team February 22, 2022 08:49

jeherve removed the [Status] In Progress label Feb 22, 2022

yansern approved these changes Feb 22, 2022

View reviewed changes

yansern added [Status] Needs Review This PR is ready for review. and removed [Status] Needs Team Review Obsolete. Use Needs Review instead. labels Feb 22, 2022

yansern requested a review from mtias February 22, 2022 12:19

Merge remote-tracking branch 'origin/master' into add/subscribe-block…

2d158e5

…-background-border-color-sync

jeherve enabled auto-merge (squash) February 22, 2022 14:16

jeherve approved these changes Feb 22, 2022

View reviewed changes

jeherve added [Status] Ready to Merge Go ahead, you can push that green button! [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it and removed [Status] Needs Review This PR is ready for review. labels Feb 22, 2022

jeherve merged commit c8b0f39 into master Feb 22, 2022

jeherve deleted the add/subscribe-block-background-border-color-sync branch February 22, 2022 14:17

github-actions bot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Feb 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Subscribe block: Add sync between button background color and border color #22949

Subscribe block: Add sync between button background color and border color #22949

Uh oh!

ivan-ottinger commented Feb 17, 2022 •

edited

Loading

Uh oh!

matticbot commented Feb 17, 2022

Uh oh!

github-actions bot commented Feb 17, 2022 •

edited

Loading

Uh oh!

yansern commented Feb 22, 2022

Uh oh!

yansern commented Feb 22, 2022 •

edited

Loading

Uh oh!

ivan-ottinger commented Feb 22, 2022 •

edited by jeherve

Loading

Uh oh!

yansern left a comment •

edited

Loading

Uh oh!

github-actions bot commented Feb 22, 2022

Uh oh!

ivan-ottinger commented Feb 23, 2022

Uh oh!

mtias commented Feb 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Subscribe block: Add sync between button background color and border color #22949

Subscribe block: Add sync between button background color and border color #22949

Uh oh!

Conversation

ivan-ottinger commented Feb 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this Pull Request:

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

Uh oh!

matticbot commented Feb 17, 2022

Uh oh!

github-actions bot commented Feb 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yansern commented Feb 22, 2022

Uh oh!

yansern commented Feb 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ivan-ottinger commented Feb 22, 2022 • edited by jeherve Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yansern left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 22, 2022

Uh oh!

ivan-ottinger commented Feb 23, 2022

Uh oh!

mtias commented Feb 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ivan-ottinger commented Feb 17, 2022 •

edited

Loading

github-actions bot commented Feb 17, 2022 •

edited

Loading

yansern commented Feb 22, 2022 •

edited

Loading

ivan-ottinger commented Feb 22, 2022 •

edited by jeherve

Loading

yansern left a comment •

edited

Loading