Skip to content

Conversation

@ockham
Copy link
Contributor

@ockham ockham commented Jul 5, 2019

Fixes #12946

Changes proposed in this Pull Request:

  • Upgrade Nudge: Fix Styling by using @wordpress/editor's Warning component

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

  • Fix styling of the recently introduced Upgrade Nudge

Screenshots

image

image

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
  • It should come with an Upgrade Nudge on top of it, which should look as depicted in the screenshots.

Proposed changelog entry for your changes:

(Covered by whatever we add for #12823)

@ockham ockham added [Status] In Progress [Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack labels Jul 5, 2019
@ockham ockham requested a review from a team July 5, 2019 04:25
@ockham ockham self-assigned this Jul 5, 2019
@matticbot
Copy link
Contributor

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 D30168-code before merging this PR. Thank you!

@jetpackbot
Copy link
Collaborator

jetpackbot commented Jul 5, 2019

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 acd632d

@matticbot
Copy link
Contributor

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

@matticbot
Copy link
Contributor

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

@matticbot
Copy link
Contributor

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

@matticbot
Copy link
Contributor

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

@ockham ockham changed the title Upgrade Nudge: Fix Styling Upgrade Nudge: Fix Styling by using @wordpress/editor's Warning component Jul 5, 2019
@ockham ockham added [Status] Needs Review This PR is ready for review. and removed [Status] In Progress labels Jul 5, 2019
@ockham ockham requested a review from a team July 5, 2019 08:42
@simison simison requested a review from mtias July 5, 2019 09:01
simison
simison previously approved these changes Jul 5, 2019
Copy link
Member

@simison simison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works well 👍

Ok to merge already, but I wanted to point out these things that you could either fix in this PR or in follow-ups:

  • There's a little off-border leak visible here:
    Screenshot 2019-07-05 at 12 35 53

  • I'd prefer to have 14px empty space between the block content and the banner:

    Screenshot 2019-07-05 at 12 43 52

    Might just need margin-bottom: 0:
    Screenshot 2019-07-05 at 12 43 48

  • I'd expect upgrade banner to get a blue hue when multiple blocks are selected:

    image

    — might be just a matter of correct z-index or background:transparent?

@ockham
Copy link
Contributor Author

ockham commented Jul 5, 2019

Also noting that the nudge doesn't look great on WP.com:

image

This is apparently because of margin styling for the .editor-styles-wrapper p selector:

image

Looks like overrides on top of overrides 😭

If I disable all 4 margin relevant checkboxes in the element inspector as seen above, the spacing looks fine:

image

@matticbot
Copy link
Contributor

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

@matticbot
Copy link
Contributor

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

@ockham
Copy link
Contributor Author

ockham commented Jul 5, 2019

Works well

Ok to merge already, but I wanted to point out these things that you could either fix in this PR or in follow-ups:

  • There's a little off-border leak visible here:
    Screenshot 2019-07-05 at 12 35 53

Fixed by e4640ff

  • I'd prefer to have 14px empty space between the block content and the banner:
    Screenshot 2019-07-05 at 12 43 52
    Might just need margin-bottom: 0:

Fixed by 0bd7b58

Screenshot 2019-07-05 at 12 43 48 * I'd expect upgrade banner to get a blue hue when multiple blocks are selected: image — might be just a matter of correct z-index or `background:transparent`?

Ideally, yeah. However, I've seen a bunch of code and/or styling in core (e.g. in the Warning component that this PR uses) that says something like 'make sure it works with multi-block selection' (or something like that), so it might be a tad more involved 😬

Edit: E.g. https://github.com/WordPress/gutenberg/blob/21e2dce2b287347e21c3e0618bc8fa8c0c1ef5ca/packages/block-editor/src/components/warning/style.scss#L11-L14 Though that's really just background:transparent 😂

@matticbot
Copy link
Contributor

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

@ockham
Copy link
Contributor Author

ockham commented Jul 5, 2019

Also noting that the nudge doesn't look great on WP.com:

image

This is apparently because of margin styling for the .editor-styles-wrapper p selector:

image

Looks like overrides on top of overrides

If I disable all 4 margin relevant checkboxes in the element inspector as seen above, the spacing looks fine:

image

Fixed by e30a44e

simison
simison previously approved these changes Jul 5, 2019
Copy link
Member

@simison simison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking very solid already!

Only things to follow up is I think fine-tuning padding/margins, e.g. there's this on hover:

Screenshot 2019-07-05 at 15 56 01

And the banner seems to jump ~1-2px when selecting the block so it doesn't feel as smooth as it does for "missing block" and similar core alerts.

Multi-selection block styles we can improve in other PR, especially if it's complicated.

Good to merge 👍

@simison simison added this to the 7.6 milestone Jul 8, 2019
@simison
Copy link
Member

simison commented Jul 8, 2019

Not needed for this PR but just noting that we might want to add this colorful border to align visually more with Calypso:

Screenshot 2019-07-08 at 09 55 35

We just need to figure out first if it's meant only for banners that are clickable and not for banners that have CTA button separately.

It might also just look silly with the thick grey border when hovering to block, let's see...

@mtias
Copy link
Member

mtias commented Jul 8, 2019

Can you try with the button vertically centered?

@matticbot
Copy link
Contributor

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

@matticbot
Copy link
Contributor

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

@simison
Copy link
Member

simison commented Jul 8, 2019

Can you try with the button vertically centered?

Fixed in 0da343a

Also upgraded the copy with a suggestion from #12946 (comment):

This block is available under the Premium Plan.
It will be hidden from site visitors until you upgrade.

The nudge looks like this now:

image

Copy link
Contributor

@zinigor zinigor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works well for me!

@zinigor 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 8, 2019
@ockham ockham merged commit 4ee3bb3 into master Jul 9, 2019
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Jul 9, 2019
@ockham ockham deleted the update/paid-block-nudge-style branch July 9, 2019 05:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Focus] Blocks Issues related to the block editor, aka Gutenberg, and its extensions developed in Jetpack Touches WP.com Files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Premium block flows: update nudge styles

8 participants