-
Notifications
You must be signed in to change notification settings - Fork 797
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 button width options for Jetpack Button and Subscriptions Form #18809
Conversation
Caution: This PR has changes that must be merged to WordPress.com |
Scheduled Jetpack release: March 2, 2021. Thank you for the great PR description! When this PR is ready for review, please apply the |
As is, this works for a button without any alignment options selected. If you choose an alignment setting from the block's menu (**important: alignment for the jetpack button, not its parent, ie Recurring Payments), the width will 'break'. This is because the alignment block support wraps the button in a floated div. I've tried applying 100% width to this wrapping div -- you can reproduce this by applying this in
This 'works' but breaks right-alignment -- the wrapping div is floated right but at 100% width, so the button inside it still sits to the left. You can fix this by adding There's a similar issue on the frontend - while no extra wrapper divs are added, the button is rendered as an anchor tag (with our desired width) inside a floated div. The biggest challenge seems to be making this work without significantly altering the way the alignment block support works. If you look to the Gutenberg Button block as an example, you'll see that it doesn't use this block support, but has its own Justification settings that don't apply well to this block. |
My second attempt is now up, which does not attempt to preserve the way the alignment controls work. Instead, I always set the wrapping div to 100% width, and then apply The consequence is that adjusting the alignment of a jetpack Button no longer causes it to be floated in the sense that you can't float it next to some other content. You can still achieve this behavior, you'll just have to use columns. If there are currently users who are relying on this floating behavior, it might break their layouts. For example, if you have a left-aligned button followed by a paragraph, this is what it would look like before applying this changeset: A similar arrangement of blocks ends up looking like this after these changes are applied: Note -- this is only the case if you've selected the alignment on the button itself. It works just as it used to if you select the alignment option on the Payments block. Ideally, I would only set that wrapping div to 100% width if the inner block has a custom width; then we could be certain that we wouldn't break any pre-existing layouts, since the width is a new feature. But there's no way to write a selector based on the presence of a child class, so I don't think we can do that without javascript. |
@stacimc 👌 Nice work, anything dealing with alignments is tricky.
Given the alignment change does affect existing content I'm not 100% sold on changing how it works. Perhaps it would be worth seeking input from more stakeholders on how big of an issue it would be. That or explore some other options to give a choice of direction to pursue. Some options might be:
Using columns would only be an approximation of the floated alignment behaviour. It's close I agree but might still frustrate users? While testing this PR I had a couple of issues or unexpected behaviour.
For the sake of discussion, I've created an alternate PR that might make my earlier suggestion on only allowing non-percentage based widths on aligned elements clearer. I ran short on time so it's by no means finished. Feel free to close it if it's not needed or run with it if it helps. |
Ah sorry for not warning you, this was meant to be very much work-in-progress still as a proof of concept while we think about approaches. There are some styling conflicts.
That's a good point. You can also adjust the alignment settings of the Recurring Payments block, although it's hard to say how many users will have used those settings as opposed to the ones on the button.
This won't break existing blocks but feels very confusing, and I think the ability to set a custom width on an aligned button is still very useful. Allowing custom px widths would be a big improvement, though it still feels strange. It also deviates even further from the behavior of the Button in Gutenberg. I'm all for adding the extra functionality (I would like to add it in Gutenberg too), but the lack of width support for aligned buttons and the floating behavior itself are fairly different. In Gutenberg the Buttons can only be justified left/right/center, not floated. From an idealistic standpoint I'd really like to get the Jetpack Button closer to its Gutenberg counterpart, and leave concerns with floating alignment to Recurring Payments. From a practical view, I do worry about breaking layouts and changing what folks are used to. Lots to think about :/ In either case, we still have to worry about alignment settings on the Recurring Payments block as well. |
It still feels like there are potential use cases where forcing the block's wrapper to be full width could be problematic. One possible shortfall in relying on the parent block's alignment could be with the contact form. If you make the button's wrapper take the full width you can't then add a paragraph to the form and have that wrap the button properly. At present, I don't believe the contact form allows columns as an inner block.
I agree completely removing the option would be confusing. I should have stated that. It was listed more for completeness. The approach in the alternate PR only removed the percentage width buttons since it's that, that really causes the problems when floated.
This is true. To play devil's advocate here, if we want the buttons to be the same as in Gutenberg perhaps we should be moving to replace Jetpack buttons with Gutenberg ones, improving the core button as needed? As you mentioned, the context in which the buttons are being used is different between Jetpack and Gutenberg. As I result I'd expect there to be some divergence in the buttons.
That sounds good in theory. We also need to ensure other blocks using the Jetpack button don't make assumptions on its capabilities. Calendly, Eventbrite, Revue etc all use it as well as Recurring Payments and Contact Form. There doesn't appear to be a perfect solution at the moment. So in the absence of that, I'd lean towards not breaking existing content and attempting to progressively enhance the features offered. |
My understanding (I could be wrong) is that the Jetpack button was meant to support additional use-cases, like submit buttons for forms, that were not desired in Gutenberg. I think what's getting to me is that to an end user not familiar with the backend, it's not necessarily clear that there are two entirely different button blocks, and that's why their styling options are different -- particularly when identical settings behave differently. To be clearer though, that's an idealistic take, and on a practical level I'm much more worried about not breaking existing content. My preference would be to get the user as many options as possible, even if we have to make some concessions. I think we're on the same page :) |
75a216a
to
0d476d8
Compare
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 If you are an automattician, once your PR is ready for review add the "[Status] Needs Team review" label and ask someone from your team review the code. jetpack plugin:
|
This is looking pretty good @stacimc. In the course of giving this another test I've found some more edge cases I think we'll need to clean up before we can get it merged. Contact FormWith the TwentyTwenty theme active and the editor window's width at 1360px or greater the button in the contact form doesn't obey the expected max width when aligned. Likely again due to it being floated. In TwentyTwentyOne, it behaves in the editor but not the frontend.
Subscriptions BlockIn the TwentyTwentyOne theme, it's a bit odd when the button obeys the width but the input doesn't however I don't believe that is caused by this PR. What it did do though is draw my eye to the width of the button within it's container and it looked misaligned. The button's wrapper gets the selected width applied to it but the margin set within the spacing controls gets applied to the actual button inside the wrapper. The result is that the button sticks out passed the boundaries of the wrapper. As those styles are being applied inline and dynamically, we could adjust the width to use A larger issue with the subscriptions block though is that the selected unit in the |
I've given this another test. Thanks for the fixes 👍 Unfortunately, it looks like I missed an issue or two in my last round of testing sorry. Subscription BlockThe subscription block's button needs a Contact FormThere also appears to be an issue with the max value on the width unit control. I found this one by:
This next one is new, I get an error thrown when inserting the contact form block now. Looks like an attempt to update state is happening during a render. |
The unit does not update automatically because updating the value prop does not cause the onChange event to fire on the UnitControl. We get around this by keeping track of the unit at the parent level until this bug can be fixed.
3e757aa
to
7371241
Compare
Rebased to get the PHP tests to pass. |
Great news! One last step: head over to your WordPress.com diff, D57003-code, and commit it. Thank you! |
r223839-wpcom |
Fixes #18594
Changes proposed in this Pull Request:
Notes:
Thanks @aaronrobertshaw for supplying this fix ^
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
Setup:
yarn build
jetpack/modules/subscriptions/views.php
to its correct location in the sandbox (mu-plugins/widgets/blog-subscription.php
)Testing:
Recurring Payments:
Repeat the above tests with the button in the Contact Form. Note that when you right-align the button in the Contact Form, it does not align it to the right side of the container. This is pre-existing behavior.
Subscriptions Form
Repeat tests with the "Place button on new line" option from the Display Settings enabled.
Resizing the Recurring Payments block and setting custom widths as %, px, em
Resizing the Contact Form button and changing alignment; custom width % is removed when the block is floated.
_Some examples of resized Subscriptions buttons_
Proposed changelog entry for your changes: