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

Add premium content button block for login and subscribe buttons #42715

Merged
merged 30 commits into from
Jun 12, 2020

Conversation

eoigal
Copy link
Contributor

@eoigal eoigal commented May 27, 2020

Changes proposed in this Pull Request

TL;DR

Changes here will offer more flexibility for the Subscribe/Login buttons allowing users:

  • Insert a Premium Content block without them.
  • Style and format them independently.
  • Insert a Premium Content login button anywhere (even outside the Premium Content block).

Full summary

  • Defines a Block Context in the container block which is consumed by the nested blocks (subscriber and non-subscriber view blocks) to know what the container's selected plan ID is and determine if those blocks should be rendered or not.
  • Decouple the login and subscribe buttons used in the non-subscriber view of the premium content block.

For the latter, this PR re-uses the existing Recurring Payment block for the subscribe button and registers a new block for the login button. That way, each button can be styled and formatted independently.

Screen Shot 2020-06-11 at 13 35 50

The Recurring Payment block automatically selects the plan selected on the Premium Content block.

The new login button block is implemented as a copy of the Core Button block but leaving out the features that are not applicable for our case (such as setting the URL).

AFAIK, the URL needs to be determined URL, so there is a small dynamic render callback that adds the href attribute to the block.

Testing instructions

  • Switch to a test site with a Recurring Payments plan created.
  • Before applying this branch, make sure you have a post published that contains a Premium Content block using the current FSE version. Try to customize the block as much as possible (i.e. button labels, background and text colors, ...).
  • Apply D44043-code or cd apps/full-site-editing && yarn dev --sync.
  • Visit the published post as a logged out user.
  • Make sure the non-subscriber view is displayed correctly (no differences with production).
  • Test that the login button, well, logs you in 😄 .
  • Test that the subscribe button opens the recurring payments modal to allow you to subscribe to the blocks plan; and confirm that when you close that modal (after you have subscribed) that you can view the premium content automatically.
  • Open the post in the block editor.
  • Test that the buttons are rendered ok in the editor (they should've been migrated to the new markup). The only visual difference should be the width (it grows now with content, previously fixed to 200px) and the margin (we use now the same margin used by the Core Buttons block).
  • Create a new post and insert a Premium Content block.
  • Test that you can remove all buttons and add them again.
  • Make sure the subscriber button block (which is a Recurring Payments block) updates its plan automatically whenever you change the selected plan on the Premium Block.
  • Publish the post.
  • Verify the subscribe/log in button continue behaving correctly for non-subscribers.
  • Create a new post and insert a Premium Content buttons block.
  • Verify you can insert a Recurring Payments, a Premium Content login button or a Core button inside the Premium Content buttons block.
  • Verify you can insert a Premium Content login button anywhere (even outside the Premium Content block or the Premium Content Buttons block).
  • Publish the post.
  • Make sure the Premium Content login button is visible only while logged out.
  • Switch to a site under a free plan.
  • Make sure neither the Premium Content buttons nor Premium Content login button can be inserted.

Fixes #41850

@matticbot
Copy link
Contributor

@eoigal eoigal self-assigned this May 27, 2020
@eoigal eoigal added Earn [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] In Progress labels May 27, 2020
@eoigal eoigal linked an issue May 27, 2020 that may be closed by this pull request
@matticbot
Copy link
Contributor

Caution: This PR affects files in the FSE Plugin on WordPress.com
Please ensure your changes work on WordPress.com before merging.

D44043-code has been created so you can easily test it on your sandbox. See this FieldGuide page about developing in the FSE Plugin for more info: PCYsg-ly5-p2

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@kwight
Copy link
Contributor

kwight commented May 27, 2020

Sweet 🎉

Test that you can remove all buttons and add them again inside the logged out (non-subscriber) view - you shouldn't be able to add a Premium content button to the subscriber view OR outside the Premium content container.

I only saw the inserter within the container. However, it would only insert a Login button – I couldn't figure out how to add back a Subscribe button. I could make multiple Login buttons though.

Copy link
Contributor

@gwwar gwwar left a comment

Choose a reason for hiding this comment

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

@eoigal would it be possible to add more before/after screenshots (or a gif/video) of what we expect for behavior? I'm a little uncertain what should happen here for manual behavior.

While testing on an existing page with the premium content block I did notice that the buttons were no longer available, so we might want to check if we need any block transformations for older versions.

@eoigal
Copy link
Contributor Author

eoigal commented May 28, 2020

While testing on an existing page with the premium content block I did notice that the buttons were no longer available, so we might want to check if we need any block transformations for older versions.

I hadn't considered that. I'll look into adding support for older versions.

@mmtr
Copy link
Member

mmtr commented May 28, 2020

I only saw the inserter within the container. However, it would only insert a Login button – I couldn't figure out how to add back a Subscribe button. I could make multiple Login buttons though.

You need to open the sidebar so you can choose the button type:

Screen Shot 2020-05-28 at 15 25 04

@eoigal
Copy link
Contributor Author

eoigal commented May 29, 2020

I've added a new function in the logged out view block that checks to see if there are any buttons in that blocks innerBlocks content. If none found, we'll dynamically insert them using the block attributes as default values - this means for posts on a older version of the plugin, they should now be able to edit the login and subscribe buttons inside the innerBlocks area and those buttons should have the same attributes as before.

I have to do something similar on the server side, so that when a post on a older version of the plugin is rendering blocks for a published post, the new button blocks will be used.

@eoigal
Copy link
Contributor Author

eoigal commented May 29, 2020

I think this is ready to be reviewed again. From my testing this PR works with posts with older versions of premium content blocks, like here.

@gwwar
Copy link
Contributor

gwwar commented May 29, 2020

Thanks @eoigal we're getting closer! Manually testing this, existing premium content blocks appear to be preserved, but I'm seeing doubles on new inserts:

Screen Shot 2020-05-29 at 2 44 36 PM

@eoigal is out till Thursday, so this PR can be taken over by @Automattic/earn or @Automattic/serenity if folks would like to help move this along.

@mmtr
Copy link
Member

mmtr commented Jun 1, 2020

I'll gave this a spin today 👍

@mmtr mmtr force-pushed the premium-content-decouple-buttons branch from c693020 to 6a46837 Compare June 1, 2020 12:49
@mmtr
Copy link
Member

mmtr commented Jun 1, 2020

I'm seeing doubles on new inserts

Took a look at that today but couldn't get a proper way of fixing it. The issue is that insertButtonBlocksIfMissing is fired too soon and the default inner blocks have not being inserted yet, causing the logic to think the block doesn't contain them. Folks are welcome to continue investigating it.

@n3f
Copy link
Contributor

n3f commented Jun 1, 2020

I've added a new function in the logged out view block that checks to see if there are any buttons in that blocks innerBlocks content. If none found, we'll dynamically insert them using the block attributes as default values

Does this mean a user can't create a block without buttons?

@gwwar
Copy link
Contributor

gwwar commented Jun 1, 2020

Does this mean a user can't create a block without buttons?

With the previous implementation, I don't think it was possible to not render the buttons.

If we decouple them from the server callback with this PR, it should be possible.

I can help take a look on Tues or Wed if folks are still stuck.

@mmtr mmtr self-assigned this Jun 2, 2020
@mmtr
Copy link
Member

mmtr commented Jun 2, 2020

Tried a different approach in b7a3a72 so the buttons are (almost) completely decoupled from the server callback, effectively addressing the duplicated buttons issues reported by @gwwar.

Summary of the changes introduced in b7a3a72:

  • Buttons are now saved to the post content duplicating most of the existing core/button block logic (so they behave and look the same).
  • For the same reasons, buttons are now wrapped in a core/buttons block.
  • The only thing that is not saved to the post content are the URLs. They are not exposed to the client, so they are added by the server-side rendering callback.
  • There is a new deprecation for the premium-content/logged-out-view block that will migrate old blocks to the new markup.
  • Existing published posts will continue rendering the buttons server-side.

There is still a few of issues we have to fix before merging this so the behavior continues matching production (namely opening the subscribe dialog with thickbox or preserving colors when migrating old blocks), but wanted to push the current state of the new approach in case folks have feedback.

@@ -0,0 +1,211 @@
// The code in this file is copied entirely from https://github.com/WordPress/gutenberg/blob/master/packages/block-library/src/button/color-edit.js
Copy link
Member

Choose a reason for hiding this comment

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

Code copied from Gutenberg, feel free to skip it during review.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe leave an issue for follow up? This feels like something we could try exporting as a npm package from the core pacakge if there's general reuse value here.

Copy link
Member

Choose a reason for hiding this comment

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

Just found out there is a plan for removing these files in favor of using the new color and style support flags. It was actually done in WordPress/gutenberg#21266 but reverted in WordPress/gutenberg#21923 due to regressions.

Will investigate if we can use those support flags to reduce code here.

Copy link
Member

Choose a reason for hiding this comment

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

Was able to get rid of those duplicate files in favor of using the color hooks in 750d3d8 🎉

@mmtr
Copy link
Member

mmtr commented Jun 3, 2020

Alright, this should be ready for review. Folks are welcome to push changes (or even revert the new approach) if needed.

@mmtr mmtr force-pushed the premium-content-decouple-buttons branch from 9c8f790 to 8012532 Compare June 12, 2020 10:12
@mmtr mmtr merged commit 0797cbc into master Jun 12, 2020
@mmtr mmtr deleted the premium-content-decouple-buttons branch June 12, 2020 10:49
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jun 12, 2020
@mmtr
Copy link
Member

mmtr commented Jun 12, 2020

Published View Has Extra Styles not reflected in Editor?

Core buttons have this issue too, so likely a theme issue or maybe a Core issue (will investigate and open an upstream issue if needed).

Turned out to be an issue in the Recurring Payments block. Should be fixed by Automattic/jetpack#16149.

When inserting premium content buttons (container) w/ multiple plans, subscribe text is replaced with X contribution

the Recurring Payments block overwrites the button label when a plan is selected and we cannot disable the behavior within the Premium Content block.

Handled in Automattic/jetpack#16148.

@gwwar
Copy link
Contributor

gwwar commented Jun 12, 2020

Kudos for pushing this one through @mmtr !

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

Successfully merging this pull request may close these issues.

Premium Content: Decouple buttons from the block
7 participants