-
Notifications
You must be signed in to change notification settings - Fork 2k
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
ETP: Premium Content: Remove __experimentalAlignmentHookSettingsProvider dep #47467
Conversation
Caution: This PR affects files in the Editing Toolkit Plugin on WordPress.com 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 |
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. |
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: [] } } |
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.
I'm not sure this line actually does anything (see PR description, section 'Open questions'). We could consider removing it.
(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
da474e0
to
31ffe93
Compare
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 Before: |
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.
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!
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.
All checks out on 9.2 and 9.3, thanks for working on this!
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:
With GB 9.3.0:
With GB 9.3.0 and this fix applied:
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
bin/gutenberg-plugin-activate.sh -e prod 9.3.0
.Verify that the same patch also works with GB 9.2.2.
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 nowdisplay: 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.
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.)I'm thus not quite sure if this was possibly broken before.