Paid Blocks Nudge: Fix Border by Injecting has-warning Style #13019

ockham · 2019-07-10T11:15:46Z

Fixes #13007.
Fixes #13008, too 😎

Changes proposed in this Pull Request:

Paid Blocks Nudge: Fix Border by Injecting has-warning Style

Prior to this PR, we used custom styling for the jetpack-paid-block__wrapper class, using padding and negative margins. This lead to visual glitches such as #13007:

As documented there, this is because that kind of styling doesn't play well with Gutenberg's own, which is using absolute positioning on :before selectors such as

.block-editor-block-list__layout .block-editor-block-list__block .block-editor-block-list__block-edit:before
.block-editor-block-list__layout .block-editor-block-list__block.is-selected>.block-editor-block-list__block-edit:before
.block-editor-block-list__layout .block-editor-block-list__block.has-warning .block-editor-block-list__block-edit:before

So ideally, we want to piggy-back on those selectors -- the has-warning ones in particular, since they do precisely what we want (also on a conceptual level). However, that's not easy from within the block, since those selectors apply to the block wrapper. Class names for that wrapper are pretty much hard-wired, and there aren't really any props to that element that we could (or should) modify to enforce the styling we want.

But fortunately, there's a filter to modify those wrappers 🎉

This PR thus uses that filter to inject the has-warning class, fixing #13007, and allowing us to drop a bunch of custom styling.

However, the has-warning selector also adds styling to prevent user interactivity with the given block (see #13019 (comment) for details), so we need to override that behavior, since we want to ensure the user can continue to interact with the block. To that end, we add another class name, is-interactive for increase specificity, and override the styling that would otherwise prevent user interactions.

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

Modifies the existing paid plan nudge.

Testing instructions:

Switch to this branch locally, build Jetpack (yarn build), and start Docker (yarn docker:up)
Now add the following line to any file in docker/mu-plugins, e.g. a newly created 0-blocks-upgrade.php (needs to start with <?php): define ( 'JETPACK_SHOW_BLOCK_UPGRADE_NUDGE', true );
Make sure you're connected to WP.com, and on a free plan
Start a new post, and insert the 'Simple Payments' block
Select the block, and enter some information into its text fields.
Unselect the block again
Verify that the visual glitch seen in the above screenshot is fixed.
Verify that you can still select and edit the block.
Finally, verify that Premium block nudge: turn banner blue when multiple blocks are selected #13008 is also fixed by this PR 🎉

Proposed changelog entry for your changes:

(Covered by whatever we add for #12823)

matticbot · 2019-07-10T11:15:54Z

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

jetpackbot · 2019-07-10T11:19:30Z

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: August 6, 2019.
Scheduled code freeze: July 30, 2019

Generated by 🚫 dangerJS against 5f9b77a

matticbot · 2019-07-10T11:20:10Z

ockham, Your synced wpcom patch D30305-code has been updated.

matticbot · 2019-07-10T11:34:37Z

ockham, Your synced wpcom patch D30305-code has been updated.

simison

I'm now getting this class for every block:

Furthermore, I cannot edit fields in simple payments block which defeats the purpose of us letting users to try it out.

ockham · 2019-07-10T12:08:43Z

Ugh my bad. I'll try to find a fix...

extensions/shared/register-jetpack-block.js

simison · 2019-07-10T12:16:08Z

Might be easiest to keep this implementation and override the background color + layer that stops pointer interactions?

matticbot · 2019-07-10T12:22:12Z

ockham, Your synced wpcom patch D30305-code has been updated.

ockham · 2019-07-10T12:33:14Z

Might be easiest to keep this implementation and override the background color + layer that stops pointer interactions?

Exactly my plan 👍

matticbot · 2019-07-10T15:26:34Z

ockham, Your synced wpcom patch D30305-code has been updated.

ockham · 2019-07-10T16:16:52Z

More fun:

The relevant styling isn't only https://github.com/WordPress/gutenberg/blob/c5e8784799f4e0f4d0891f52fae95375f43b76f7/packages/block-editor/src/components/block-list/style.scss#L219-L224

but also https://github.com/WordPress/gutenberg/blob/c5e8784799f4e0f4d0891f52fae95375f43b76f7/packages/block-editor/src/components/block-list/style.scss#L252-L261 which generates a transparent pseudo-element that eats up cursor focus, and tints its what's behind it grey.

matticbot · 2019-07-10T16:51:36Z

ockham, Your synced wpcom patch D30305-code has been updated.

Issue found in review should be fixed now

extensions/shared/with-has-warning-is-interactive-class-names/style.scss

simison · 2019-07-11T06:53:11Z

extensions/shared/register-jetpack-block.js

+	if ( requiredPlan ) {
+		addFilter(
+			'editor.BlockListBlock',
+			`jetpack/${ name }-with-has-warning-is-interactive-class-names`,


Haha that's a mouthful!

Yeah, same for the component name 😬 Happy to change it something more palatable!

simison

Looking good and works fabulously well!

simison · 2019-07-11T07:51:07Z

extensions/shared/with-has-warning-is-interactive-class-names/index.jsx

+		BlockListBlock => props => (
+			<BlockListBlock
+				{ ...props }
+				className={ props.name === name ? 'has-warning is-interactive' : '' }


is-interactive isn't a core class, right? Therefore I'd jetpack-prefix it. Not a blocker tho.

zinigor

Works well for me, the border appears as it should.

Paid Blocks Nudge: Fix Border by Injecting has-warning Style

1e2edcc

ockham added [Status] Needs Review This PR is ready for review. [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack labels Jul 10, 2019

ockham requested review from a team July 10, 2019 11:15

ockham self-assigned this Jul 10, 2019

matticbot added the Touches WP.com Files label Jul 10, 2019

wrapPaidBlock: Move out of directory

e2e917d

ockham mentioned this pull request Jul 10, 2019

Premium block nudge: turn banner blue when multiple blocks are selected #13008

Closed

Add explanatory comment

89f1d44

simison previously requested changes Jul 10, 2019

View reviewed changes

simison reviewed Jul 10, 2019

View reviewed changes

extensions/shared/register-jetpack-block.js Outdated Show resolved Hide resolved

Apply class only to paid blocks

dc630d9

Fix filter name

15adebb

Make Blocks Interactive Again

0c60443

ockham requested a review from simison July 10, 2019 17:01

simison reviewed Jul 11, 2019

View reviewed changes

extensions/shared/with-has-warning-is-interactive-class-names/style.scss Show resolved Hide resolved

simison reviewed Jul 11, 2019

View reviewed changes

simison previously approved these changes Jul 11, 2019

View reviewed changes

simison reviewed Jul 11, 2019

View reviewed changes

Add a note to styles

5f9b77a

simison dismissed their stale review via 5f9b77a July 11, 2019 07:52

zinigor approved these changes Jul 11, 2019

View reviewed changes

zinigor added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Jul 11, 2019

ockham merged commit 78ed77c into master Jul 12, 2019

ockham deleted the fix/paid-block-border branch July 12, 2019 06:19

matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Jul 12, 2019

jeherve added this to the 7.6 milestone Jul 12, 2019

jeherve removed the [Status] Needs Changelog label Jul 12, 2019

Paid Blocks Nudge: Fix Border by Injecting has-warning Style #13019

Paid Blocks Nudge: Fix Border by Injecting has-warning Style #13019

Uh oh!

Conversation

ockham commented Jul 10, 2019 • 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 Jul 10, 2019

Uh oh!

jetpackbot commented Jul 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matticbot commented Jul 10, 2019

Uh oh!

matticbot commented Jul 10, 2019

Uh oh!

simison left a comment

Choose a reason for hiding this comment

Uh oh!

ockham commented Jul 10, 2019

Uh oh!

Uh oh!

simison commented Jul 10, 2019

Uh oh!

matticbot commented Jul 10, 2019

Uh oh!

ockham commented Jul 10, 2019

Uh oh!

matticbot commented Jul 10, 2019

Uh oh!

ockham commented Jul 10, 2019

Uh oh!

matticbot commented Jul 10, 2019

Uh oh!

Uh oh!

simison Jul 11, 2019

Choose a reason for hiding this comment

Uh oh!

ockham Jul 11, 2019

Choose a reason for hiding this comment

Uh oh!

simison left a comment

Choose a reason for hiding this comment

Uh oh!

simison Jul 11, 2019

Choose a reason for hiding this comment

Uh oh!

zinigor left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ockham commented Jul 10, 2019 •

edited

Loading

jetpackbot commented Jul 10, 2019 •

edited

Loading