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.

Sign up for GitHub

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

Jump to bottom

Abstract button block width settings in a new useWidth hook #25394

Merged
merged 12 commits into from
Aug 11, 2022
Merged

Abstract button block width settings in a new useWidth hook #25394

merged 12 commits into from
Aug 11, 2022

Conversation

mmtr
Copy link
Member

@mmtr mmtr commented Aug 5, 2022 •
edited
Loading

Required by #25297

Changes proposed in this Pull Request:

This PR abstracts the button width settings in a new useWidth hook so they can be used by any other block with a width attribute. This will helps us to implement a Payment buttons block container since we'll need to control the width of the inner Payment button blocks.

This was initially implemented in #25297, but I decided to open a separate PR to facilitate the review.

Other information:

  • Have you written new tests for your changes, if applicable?
  • Have you checked the E2E test CI results, and verified that your changes do not break them?

Jetpack product discussion

No.

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

N/A.

Testing instructions:

Since this PR doesn't introduce any behavior or aesthetic change, you mainly need to double check that there are no regressions for any block that uses the Button block.

  • Spin up a Jurassic Ninja site running this branch.
  • Set up Jetpack.
  • Go to /wp-admin/post-new.php.
  • Insert a Payment Button block.
  • Make sure you can change the width of the button as usual.
  • Insert a Form block.
  • Choose Contact Form.
  • Make sure you can change the width of the submit button as usual.

@mmtr mmtr requested a review from a team August 5, 2022 08:45
@mmtr mmtr self-assigned this Aug 5, 2022
@github-actions github-actions bot added [Block] Button [Block] Subscriptions [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ labels Aug 5, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Aug 5, 2022 •
edited
Loading

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • ⚠️ All commits were linted before commit.
  • ✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • ✅ Add changelog entries to affected projects

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 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.

Jetpack plugin:

  • Next scheduled release: September 6, 2022.
  • Scheduled code freeze: August 30, 2022.

@github-actions github-actions bot added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Aug 5, 2022
@mmtr mmtr removed the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Aug 5, 2022
@mmtr mmtr force-pushed the update/payment-button-block-width branch from cffc6f6 to e6c0671 Compare August 5, 2022 08:54
Base automatically changed from migrate/recurring-payments-block-api-v2 to trunk August 5, 2022 13:06
@github-actions github-actions bot added the [Block] Payments aka Unified Intro label Aug 5, 2022
@mmtr mmtr force-pushed the update/payment-button-block-width branch from e6c0671 to a7088ec Compare August 5, 2022 13:15
@matticbot
Copy link
Contributor

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

@mmtr mmtr added this to the jetpack/11.3 milestone Aug 5, 2022
dsas
dsas reviewed Aug 11, 2022
View reviewed changes
Copy link
Contributor

@dsas dsas left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Edit: This is a thing, but it's not a regression - it failed like this before

When changing the alignment, the width gets reset and the width selector changes so that the buttons are no longer available

Screen.Capture.on.2022-08-11.at.12-10-41.mp4

@dsas
Copy link
Contributor

dsas commented Aug 11, 2022

Hmm, actually the same happens before, it's not a regression.

@mmtr
Copy link
Member Author

mmtr commented Aug 11, 2022

I think that's actually an intended behavior introduced by #18809:

  • When left or right alignment is selected on the Jetpack button block, the block is placed in a floated container. This prevents percentage widths from working correctly. We handle this by:
    • The % presets and custom % width options are removed from the controls when left/right alignment is selected
    • If a button already has a % based width applied, and then the user selects left/right alignment, the custom width is removed. They are still able to apply a px/em width.

@mmtr mmtr added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] Needs Team Review labels Aug 11, 2022
Copons
Copons approved these changes Aug 11, 2022
View reviewed changes
Copy link
Contributor

@Copons Copons left a comment

Choose a reason for hiding this comment

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

LGTM!

@Copons Copons added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Aug 11, 2022
@mmtr mmtr merged commit 9d58683 into trunk Aug 11, 2022
@mmtr mmtr deleted the update/payment-button-block-width branch August 11, 2022 12:59
@github-actions github-actions bot removed the [Status] Ready to Merge Go ahead, you can push that green button! label Aug 11, 2022
@github-actions
Copy link
Contributor

Great news! One last step: head over to your WordPress.com diff, D85445-code, and deploy it.
Once you've done so, come back to this PR and add a comment with your SVN changeset ID (e.g. r12345-wpcom).

Thank you!

@mmtr
Copy link
Member Author

mmtr commented Aug 11, 2022

r250534-wpcom

@mmtr mmtr restored the update/payment-button-block-width branch August 11, 2022 13:08
@mmtr mmtr deleted the update/payment-button-block-width branch August 11, 2022 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Copons Copons Copons approved these changes

@dsas dsas dsas approved these changes

Assignees

@mmtr mmtr

Labels
[Block] Button [Block] Payments aka Unified Intro [Block] Subscriptions [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ Touches WP.com Files
Projects
None yet
Milestone
jetpack/11.3
Development

Successfully merging this pull request may close these issues.
4 participants
@mmtr @matticbot @dsas @Copons