-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Conversation
Caution: This PR affects files in the FSE Plugin on WordPress.com 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 |
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. |
Sweet 🎉
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. |
There was a problem hiding this 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.
apps/full-site-editing/full-site-editing-plugin/premium-content/blocks/button/edit.js
Outdated
Show resolved
Hide resolved
apps/full-site-editing/full-site-editing-plugin/premium-content/blocks/button/edit.js
Outdated
Show resolved
Hide resolved
I hadn't considered that. I'll look into adding support for older versions. |
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. |
apps/full-site-editing/full-site-editing-plugin/premium-content/blocks/button/edit.js
Outdated
Show resolved
Hide resolved
apps/full-site-editing/full-site-editing-plugin/premium-content/blocks/button/index.js
Outdated
Show resolved
Hide resolved
apps/full-site-editing/full-site-editing-plugin/premium-content/blocks/button/edit.js
Outdated
Show resolved
Hide resolved
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. |
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: @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. |
I'll gave this a spin today 👍 |
c693020
to
6a46837
Compare
Took a look at that today but couldn't get a proper way of fixing it. The issue is that |
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. |
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:
|
@@ -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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 🎉
apps/full-site-editing/full-site-editing-plugin/premium-content/blocks/button/color-props.js
Outdated
Show resolved
Hide resolved
Alright, this should be ready for review. Folks are welcome to push changes (or even revert the new approach) if needed. |
… can still be inserted anywhere
9c8f790
to
8012532
Compare
Turned out to be an issue in the Recurring Payments block. Should be fixed by Automattic/jetpack#16149.
Handled in Automattic/jetpack#16148. |
Kudos for pushing this one through @mmtr ! |
Changes proposed in this Pull Request
TL;DR
Changes here will offer more flexibility for the Subscribe/Login buttons allowing users:
Full summary
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.
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
cd apps/full-site-editing && yarn dev --sync
.Fixes #41850