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

ETP: Premium Content: Remove __experimentalAlignmentHookSettingsProvider dep #47467

Merged
merged 2 commits into from
Nov 17, 2020

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Nov 16, 2020

Changes proposed in this Pull Request

Premium Content blocks would be broken by the GB 9.3 upgrade on WP.com. This PR attempts a fix.
The reason for the breakage is that __experimentalAlignmentHookSettingsProvider was removed in WordPress/gutenberg#26380.

With GB 9.2.2:

image

With GB 9.3.0:

image

With GB 9.3.0 and this fix applied:

image

The changed layout is discussed in the 'Questions' section at the bottom of this PR description. I would like to land this PR regardless of whether or not we manage to fix the layout, as it would otherwise block us from upgrading Gutenberg versions altogether. A fix for the layout could be attempted in a follow-up PR.

The changes in this PR are largely modeled after this file in aforementioned PR: https://github.com/WordPress/gutenberg/pull/26380/files#diff-4347ded500d1f93237e9e254af2e2f5a01896b5d1736c61f965f6d333847f8bd.

Testing instructions

  • Choose a WP.com Simple Site for testing. It should have the Premium Content block enabled. As that requires a bit of doing, you can ask team #good-mountain for a test site.
  • Sandbox that site, and the REST API.
  • Open a post that contains the Premium Content block in the post editor, and verify that it works.
  • Update that site's block editor to 9.3.0 by running the following command in your sandbox: bin/gutenberg-plugin-activate.sh -e prod 9.3.0.
  • Reload the post that you used for testing, and verify that the block is now broken (with an error message as seen in the screenshot above).
  • Apply the WP.com companion patch (D52816-code) to your site.
  • Reload your test post once more, and verify that the block is working again.

Verify that the same patch also works with GB 9.2.2.

  • To that end, revert the version bump: bin/gutenberg-plugin-activate.sh -e prod 9.2.2, reload your test post, and proceed testing.

Questions

Changed Layout

Open question: The layout has changed (buttons appear on top of each other, instead of next to each other, as desired). Inspection of DOM elements revealed that because of the .wp-block-buttons class, there's now display: flex (and a number of flex-related styles) applied, which previously weren't (even though the <div /> had the same class) 🤔
Edit: I think those are coming from here, which was last touched by #23168. We might have to study that PR and actually might need to pass a higher-level attribute/settings to the Buttons inner block here.

image

Alignment toolbar removal

It seems that the __experimentalAlignmentHookSettingsProvider stuff was originally used to suppress the alignment toolbar from the block (at least WordPress/gutenberg#26380 suggests that that's what it was mostly used for). As the setting was applied to the Premium Content buttons inner block, I was assuming that it would apply to them; however, I was able to see the alignment toolbar option in production. (Note that the toolbar seems to be attached to the parent Premium Content block rather than to the child Premium Content buttons block; that glitch is also pre-existing.)

image

I'm thus not quite sure if this was possibly broken before.

@ockham ockham added [Status] In Progress Premium Content Controlling specific content for paying site visitors. Editing Toolkit For issues and PRs that affect the Editing Toolkit plugin labels Nov 16, 2020
@ockham ockham requested a review from sirreal November 16, 2020 16:47
@ockham ockham self-assigned this Nov 16, 2020
@matticbot
Copy link
Contributor

@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 16, 2020
@matticbot
Copy link
Contributor

Caution: This PR affects files in the Editing Toolkit Plugin on WordPress.com
Please ensure your changes work on WordPress.com before merging.

D52816-code has been created so you can easily test it on your sandbox. See this FieldGuide page about developing the Editing Toolkit Plugin for more info: PCYsg-ly5-p2

@matticbot
Copy link
Contributor

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.

@ockham
Copy link
Contributor Author

ockham commented Nov 16, 2020

Holding off on merging this because there's a number of other undeployed ETP changes in #47442, and we're not confident we might break other things by landing those.

allowedBlocks={ ALLOWED_BLOCKS }
template={ isPreview ? previewTemplate : template }
templateInsertUpdatesSelection={ false }
__experimentalLayout={ { type: 'default', alignments: [] } }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure this line actually does anything (see PR description, section 'Open questions'). We could consider removing it.

@ockham ockham marked this pull request as ready for review November 17, 2020 11:17
@ockham ockham requested review from a team November 17, 2020 11:17
@sirreal
Copy link
Member

sirreal commented Nov 17, 2020

(unrelated) I couldn't get this building locally and opened #47498 to fix the problem.

Gutenberg removed display: inline-block.
Restore to fix visual regression
@sirreal sirreal force-pushed the fix/etp-remove-exp-alignment-hook-settings-provider branch from da474e0 to 31ffe93 Compare November 17, 2020 13:20
@sirreal
Copy link
Member

sirreal commented Nov 17, 2020

I've pushed 31ffe93 to fix the visual regression. This appears to be caused by a different structure in the editor.

Gutenberg includes a rule like .wp-block-buttons>.wp-block-button { display: inline-block } which appears to apply on the frontend but not in the editor. The layout of this block depended on that inline-block so I've applied it to the button that was misplaced.

Before:

Screen Shot 2020-11-17 at 14 18 54

After
Screen Shot 2020-11-17 at 14 18 38

Copy link
Member

@sirreal sirreal left a comment

Choose a reason for hiding this comment

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

I've been testing this with pcz0P1-r-p2 and it works well on the frontend and in the editor, with 9.2 and 9.3.

Thanks!

@sirreal sirreal added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] In Progress labels Nov 17, 2020
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.

All checks out on 9.2 and 9.3, thanks for working on this!

@ockham ockham merged commit a12504b into master Nov 17, 2020
@ockham ockham deleted the fix/etp-remove-exp-alignment-hook-settings-provider branch November 17, 2020 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Editing Toolkit For issues and PRs that affect the Editing Toolkit plugin Premium Content Controlling specific content for paying site visitors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants