Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Block Style Previews: Fix block style previews with an upgrade nudge #14730

Merged
merged 2 commits into from
Feb 20, 2020
Merged

Block Style Previews: Fix block style previews with an upgrade nudge #14730

merged 2 commits into from
Feb 20, 2020

Conversation

pablinos
Copy link
Contributor

When an upgrade nudge is to be displayed, the block example setting is
removed and this threw up a bug where we were calling createBlock with the
incorrect arguments.

With that fixed, it still meant that the block previews had the nudge
rendered.

Changes proposed in this Pull Request:

This change fixes the createBlock bug and prevents the nudge from rendering in the preview by defining an additional attribute on the created block.

The nudge then isn't displayed if that attribute is found to be true. I realise that this is somewhat hacky, but I'm not sure of a better way of passing that information down to the wrapPaidBlock component. We could store something in the state somewhere instead?

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

This is a bug fix

Testing instructions:

Running this branch on a sandboxed site (probably easiest using the Fusion diff) create an OpenTable or Calendly block on a site with a free plan. Check that the nudge is displayed and when the block is selected, that you can see the previews of the various styles.

Proposed changelog entry for your changes:

Bug fixed where incorrect parameters were passed to createBlock when generating a block style preview

@pablinos
Fix block style previews with an upgrade nudge
95af514 
When an upgrade nudge is to be displayed, the block `example` setting is
removed and threw up a bug where we were calling `createBlock` with the
incorrect arguments.

With that fixed it still meant that the block previews had the nudge
rendered. This change gets around that by defining an additional
attribute an the created block, and not displaying the nudge if that
attribute is found to be `true`.
@pablinos pablinos added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Block] OpenTable [Block] Calendly labels Feb 19, 2020
@pablinos pablinos requested a review from a team February 19, 2020 16:36
@pablinos pablinos self-assigned this Feb 19, 2020
@matticbot
Copy link
Contributor

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

@jetpackbot
Copy link

jetpackbot commented Feb 19, 2020 •
edited
Loading

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 72dd904

@scruffian
Copy link
Member

This fix works, but I'm not convinced its the right approach. When I add an upgrade nudge to the Tiled Gallery block, the core style selector works fine, with the upgrade nudge in the preview. There's obviously some difference between the way our BlockStyleSelector component works and the way it's working in core, but I haven't got to the bottom of it yet.

@scruffian scruffian mentioned this pull request Feb 19, 2020
Merged
@jeherve jeherve added the [Type] Bug When a feature is broken and / or not performing as intended label Feb 19, 2020
@jeherve jeherve added this to the 8.3 milestone Feb 19, 2020
simison
simison reviewed Feb 19, 2020
View reviewed changes
extensions/shared/components/block-styles-selector/index.js Outdated
...block,
attributes: {
...block.attributes,
isBlockPreview: true,
Copy link
Member

Choose a reason for hiding this comment

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

Since this is kinda "internal" attribute, should it have underscore prefix to indicate it; __isblockPreview ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good idea, I'll update that

@pablinos
Copy link
Contributor Author

BlockStyleSelector component works and the way it's working in core

Yes, the core one, doesn't have this fix in it, so the nudge will still display in the generated preview, or are you seeing something else @scruffian?

@matticbot
Copy link
Contributor

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

scruffian
scruffian approved these changes Feb 20, 2020
View reviewed changes
Copy link
Member

@scruffian scruffian left a comment

Choose a reason for hiding this comment

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

Good job! 🚢

jeherve
jeherve approved these changes Feb 20, 2020
View reviewed changes
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This seems to test well in Jetpack and on WordPress.com. 👍

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Feb 20, 2020
@scruffian scruffian merged commit ef97833 into master Feb 20, 2020
@scruffian scruffian deleted the fix/style-preview-nudge branch February 20, 2020 17:24
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Feb 20, 2020
@scruffian
Copy link
Member

r203152-wpcom

jeherve added a commit that referenced this pull request Feb 25, 2020
@jeherve
Changelog: add #14730
567e8dc
jeherve added a commit that referenced this pull request Feb 25, 2020
@jeherve
Changelog: 8.3 running changelog (#14642)
8797bea 
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simison simison simison left review comments

@jeherve jeherve jeherve approved these changes

@scruffian scruffian scruffian approved these changes

Assignees

@pablinos pablinos

Labels
[Block] Calendly [Block] OpenTable Touches WP.com Files [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Milestone
8.3
Development

Successfully merging this pull request may close these issues.
6 participants
@pablinos @matticbot @jetpackbot @scruffian @simison @jeherve