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.

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

Payments Block: Remove upgrade nudges when used in Premium Content block #17702

Merged

Conversation

apeatling
Copy link
Member

@apeatling apeatling commented Nov 3, 2020

Changes proposed in this Pull Request:

Remove all native upgrade nudges inside the Payments block when used within the Premium Content block. This stops the duplication of nudges when used together. This also helps us when using these blocks in patterns so the canvas is cleaner.

Partially fixes Automattic/wp-calypso#46923

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

No

Testing instructions:

Proposed changelog entry for your changes:

  • None needed.

@apeatling apeatling added the [Block] Payment Button aka Recurring Payments label Nov 3, 2020
@apeatling apeatling self-assigned this Nov 3, 2020
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello apeatling! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D52237-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

@apeatling apeatling changed the title Payments Block: Remove upgrade nudges when used in Premium Content block [WIP] Payments Block: Remove upgrade nudges when used in Premium Content block Nov 3, 2020
@jetpackbot
Copy link

jetpackbot commented Nov 3, 2020

Warnings
⚠️

pre-commit hook was skipped for one or more commits

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against aefc05d

@apeatling apeatling added the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Nov 3, 2020
@apeatling apeatling changed the title [WIP] Payments Block: Remove upgrade nudges when used in Premium Content block Payments Block: Remove upgrade nudges when used in Premium Content block Nov 3, 2020
@apeatling
Copy link
Member Author

I need to check these options are still available when using this within the premium content block, now that the placeholder is hidden there

image

@glendaviesnz
Copy link
Contributor

I get some weird layout caused by the payments disclaimer block (seedlet theme):

Screen Shot 2020-11-06 at 10 51 39 AM

@apeatling
Copy link
Member Author

I get some weird layout caused by the payments disclaimer block (seedlet theme):

This disclaimer is being moved to the sidebar in #17696 so that should clean up this visual issue. 👍

@glendaviesnz
Copy link
Contributor

This seems to work for me. With both diffs applied to sandbox I just see one upgrade nudge at the top, and content and subscribe buttons are visible:

Screen Shot 2020-11-10 at 9 01 47 AM

I also see this same nudge in centre of block if the payment parent block is selected - is that correct?

Screen Shot 2020-11-10 at 9 02 21 AM

@apeatling
Copy link
Member Author

I also see this same nudge in centre of block if the payment parent block is selected - is that correct?

Yes this won't hide the global upgrade nudge UI, just any placeholders local to the block that ask you to upgrade.

glendaviesnz
glendaviesnz previously approved these changes Nov 9, 2020
Copy link
Contributor

@glendaviesnz glendaviesnz left a comment

Choose a reason for hiding this comment

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

LGTM

@apeatling apeatling requested a review from a team November 9, 2020 20:39
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.

Insert a premium content block

This must be a stupid question, but how do I go about doing that? I can't seem to see a Premium Content block on my WordPress.com simple sites.

image


Maybe this would be an opportunity to move away from the custom upgrade nudge of the Payments block, and switch to using the standard upgrade nudges like all the other blocks have? This is documented in #14893. That may allow us to fix that issue for all parent/child blocks at once, directly in the upgrade banner logic in extensions/extended-blocks/paid-blocks/.
What do you think?

@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Nov 11, 2020
@apeatling
Copy link
Member Author

This must be a stupid question, but how do I go about doing that? I can't seem to see a Premium Content block on my WordPress.com simple sites.

@jeherve I believe there are issues with the way Premium Content manages this, it's not in Jetpack so it has duplicated and forked a lot of the nudge and restriction code. It shows on some of my free sites, but not all. What we plan to do is move this block into Jetpack in the coming weeks so we can remove all the duplicated code and unify it with the Jetpack implementation.

Okay to move forward with merging this and addressing that as a followup?

@ockham
Copy link
Contributor

ockham commented Nov 16, 2020

What we plan to do is move this block into Jetpack in the coming weeks so we can remove all the duplicated code and unify it with the Jetpack implementation.

@apeatling This is great news, thanks a lot for taking this on! 🎉 One request, could y'all maybe add an issue to your team's board to add some basic e2e test coverage for that block in order to guard against regressions like the one we're hoping to fix with Automattic/wp-calypso#47467? There's some precedent of e2e tests for paid blocks in JP already, and e2e tests are run against the latest GB plugin version, so the basic infrastructure should be in place already.

@apeatling apeatling added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Nov 22, 2020
@apeatling apeatling added the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Nov 24, 2020
@apeatling apeatling requested a review from jeherve November 24, 2020 19:41
@jeherve jeherve added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Nov 25, 2020
@apeatling apeatling force-pushed the remove/upgrade-notice-for-payments-premium-content-block branch from cad2723 to deffcd4 Compare December 3, 2020 22:40
@apeatling apeatling added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Dec 3, 2020
@apeatling
Copy link
Member Author

@jeherve Good to merge this one?

@jeherve jeherve force-pushed the remove/upgrade-notice-for-payments-premium-content-block branch from deffcd4 to 08248ba Compare December 9, 2020 10:39
@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 Dec 9, 2020
@jeherve jeherve added this to the 9.3 milestone Dec 9, 2020
jeherve
jeherve previously approved these changes Dec 9, 2020
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.

All good after a rebase to address unrelated test failures.

@apeatling apeatling force-pushed the remove/upgrade-notice-for-payments-premium-content-block branch from 08248ba to aefc05d Compare December 9, 2020 16:32
@apeatling
Copy link
Member Author

@jeherve Thanks, rebased. just need another approval here to merge.

@apeatling apeatling merged commit 97d625e into master Dec 9, 2020
@apeatling apeatling deleted the remove/upgrade-notice-for-payments-premium-content-block branch December 9, 2020 17:47
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Dec 9, 2020
@jeherve
Copy link
Member

jeherve commented Jan 14, 2021

r219427-wpcom

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Premium Content Block: Visual issues with upgrade and connection notices
6 participants