-
Notifications
You must be signed in to change notification settings - Fork 800
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
Recurring Payments: Replace submit button #15915
Recurring Payments: Replace submit button #15915
Conversation
Caution: This PR has changes that must be merged to WordPress.com |
Thank you for the great PR description! When this PR is ready for review, please apply the E2E results is available here (for debugging purposes): https://jetpack-e2e-dashboard.herokuapp.com/pr-15915 Scheduled Jetpack release: August 4, 2020. |
This block apparently allows for custom button HTML attributes from WPCOM VIP sites. I don't know how to test with VIP sites or even how their versions of this block might differ. If anyone can point me in the right direction of what needs doing, that would be great. |
5115bc6
to
fd2921f
Compare
I've tested this and it works as advertised. I'll look at the code tomorrow 👍 |
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 changes LGTM 👍
8fdc722
to
e21d5a7
Compare
@glendaviesnz I've rebased this PR and resolved the conflict. It could use another quick test to have someone else confirm the custom button text remains when switching plans for the block. I've updated the test instructions and tested the block myself and it's working for me. |
Now we have a shared Button component we can replace the SubmitButton used in some blocks to provide more consistent and additional features.
It turned out that all the VIP uses of the button just called render button directly passing attributes. As these won't pass inner block content they are treated as deprecated buttons for rendering. This means we won't be getting submitButtonAttributes while rendering the newest versions of the block added through the editor.
e21d5a7
to
c6f45df
Compare
Hi @fgiannar , Thanks for testing this one. Not being easily able to select the parent block while the inner button block is aligned left or right is an issue. It is a side-effect of the switch to using inner blocks. That said, we do have the block navigation button in the main editor toolbar as well as the parent selector when hovering the button block’s toolbar. There is also currently work in progress ( PR ) to improve the UX around the parent selector, making it more discoverable and obvious. If you are happy with these options in the short term, I’d be inclined to proceed with this PR. What do you think? |
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.
Hi @aaronrobertshaw ,
Thanks for the rebase.
I think, as you mentioned, not being easily able to select the parent block while the inner button block is aligned left or right is a generic issue that needn't affect this PR.
I believe you can proceed.
r211585-wpcom |
Fixes #15713
Changes proposed in this Pull Request:
Does this pull request change what data or activity we track or use?
No.
Prerequisites
public-api.wordpress.com
andsubscribe.wordpress.com
. See instructions for sandboxing for Recurring Payments in the Field Guide.Testing instructions:
Button should still be contained within div with
.wp-block-button
class.Before:
After:
Proposed changelog entry for your changes: