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

Subscriptions Block: Update subscriptions block to handle CSS unit font sizes #18936

Merged

Conversation

aaronrobertshaw
Copy link
Contributor

@aaronrobertshaw aaronrobertshaw commented Feb 25, 2021

Fixes #18250

Changes proposed in this Pull Request:

  • Updates subscription block font size attributes to be strings containing CSS units
  • Updates deprecations to migrate numeric pixel only versions to the new format

Jetpack product discussion

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

No changes.

Testing instructions:

  1. Load dev site from Jetpack master with Gutenberg plugin installed and active
  2. Create a post, add a subscriptions block and modify the block's font size
  3. Save the post and reload the editor, you'll receive a validation error
  4. Checkout this PR
  5. Reload the post and the block should be migrated
  6. Tweak the font size again and save
  7. Reload the editor and ensure no further block validation errors
  8. Check the block still works on the frontend
Note if syncing to wpcom via a8c-wp-env you'll need to manually sync the views.php file to the correct location. One easy way to do this would be to copy the sync'd file to the different location and name the file exists under on wpcom.

cp ~/public_html/wp-content/mu-plugins/jetpack/modules/subscriptions/views.php ~/public_html/wp-content/mu-plugins/widgets/blog-subscription.php

Screenshots

Before After
SubscriptionsFontSize-Before SubscriptionsFontSize-After

Proposed changelog entry for your changes:

  • Bug Fix: Prevent Subscription block validation error when customizing font size.

Due to Gutenberg updating its FontSizePicker and theme.json to handle
CSS units, those values are now strings which were incompatible with
the approach in the subscriptions block shortcode for custom font
sizes.
@aaronrobertshaw aaronrobertshaw added [Type] Bug When a feature is broken and / or not performing as intended [Status] Needs Review To request a review from Crew. Label will be renamed soon. [Block] Subscriptions labels Feb 25, 2021
@aaronrobertshaw aaronrobertshaw requested a review from a team February 25, 2021 11:21
@aaronrobertshaw aaronrobertshaw self-assigned this Feb 25, 2021
@matticbot
Copy link
Contributor

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

@github-actions github-actions bot added [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Feature] Subscriptions All subscription-related things such as paid and unpaid, user management, and newsletter settings. labels Feb 25, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Feb 25, 2021

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.
  • ✅ Include a changelog entry for any meaningful change.
  • ✅ Specify whether this PR includes any changes to data or privacy.

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.
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.undefined


jetpack plugin:

  • Next scheduled release: April 6, 2021.
  • Scheduled code freeze: March 29, 2021

@jeherve jeherve added [Status] Needs Team Review and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Feb 25, 2021
@andrewserong
Copy link
Member

@aaronrobertshaw this is testing okay for me in the editor, however the custom font size doesn't appear to be showing up on the front end of the site for me (it always renders at the one size, irrespective of the setting I've used in the editor). Is it working for you? It's possible I might be missing a step in my testing setup! (Testing synced to wpcom)

With Barnsbury theme activated

Editor view Front end view
image image

With Twenty Twenty One activated

With Twenty Twenty One, the width of the text field and the background of the submit button don't appear to be rendering correctly on the front end either (but that doesn't appear to be caused by this change). I believe I encountered the same issue as the one you raised in: #18809 (comment) so not related to this PR per se.

Editor view Front end view
image image

@aaronrobertshaw
Copy link
Contributor Author

Thanks for the review @andrewserong!

this is testing okay for me in the editor, however the custom font size doesn't appear to be showing up on the front end of the site for me (it always renders at the one size, irrespective of the setting I've used in the editor). Is it working for you? It's possible I might be missing a step in my testing setup! (Testing synced to wpcom)

Sounds like you are testing by using the a8c-wp-env sync to your sandbox? Unfortunately, that sync doesn't the subscription block's views.php file correctly. On wpcom that file resides in a different location under a different name. The wpcom diff, D57652-code, will handle that or you can manually sync or copy the file into the correct location.

I've updated the PR's test instructions. It is also testing correctly for me on the frontend with that sync'ing issue accounted for.

I believe I encountered the same issue as the one you raised in: #18809 (comment) so not related to this PR per se.

Yep, there are a few quirks about this block in terms of styles/layout. I think it would be good to revisit this block after #18809 and this PR land. Then via a new issue we can find a solution that takes into account both the custom button widths and font sizes.

Copy link
Member

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

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

This is testing nicely for me on the front end too now, thanks for the updated testing instructions! I'm now able to set a ridiculously big font-size: 😄

image

Also confirmed that the migration from an invalid block appears to be working correctly, too. 👍

@andrewserong andrewserong added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] Needs Team Review labels Mar 1, 2021
@apeatling apeatling self-requested a review March 1, 2021 23:37
Copy link
Member

@apeatling apeatling left a comment

Choose a reason for hiding this comment

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

Working well for me!

Copy link
Contributor

@leogermani leogermani left a comment

Choose a reason for hiding this comment

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

Tested well for me. Block was correctly migrated and everything worked as expected. Looks good to go.

@leogermani leogermani added this to the 9.6 milestone Mar 2, 2021
@leogermani leogermani 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 Mar 2, 2021
@aaronrobertshaw aaronrobertshaw merged commit 92a66a4 into master Mar 3, 2021
@aaronrobertshaw aaronrobertshaw deleted the fix/subscription-font-size-block-validation-errors branch March 3, 2021 00:17
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Mar 3, 2021
@jeherve jeherve modified the milestones: 9.6, 9.5.1 Mar 3, 2021
jeherve pushed a commit that referenced this pull request Mar 3, 2021
…es (#18936)

Due to Gutenberg updating its FontSizePicker and theme.json to handle
CSS units, those values are now strings which were incompatible with
the approach in the subscriptions block shortcode for custom font
sizes.
@jeherve
Copy link
Member

jeherve commented Mar 3, 2021

Cherry-picked to jetpack/branch-9.5 in d8b5865

@jeherve
Copy link
Member

jeherve commented Mar 3, 2021

r222019-wpcom

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Subscriptions [Feature] Subscriptions All subscription-related things such as paid and unpaid, user management, and newsletter settings. [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ Touches WP.com Files [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Subscriptions Block: font size cannot be modified
6 participants