[RNMobile] Buttons block: Fix content justification attribute #37887
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The only hook currently supported by the native version is "core/layout/addAttribute", which extends block attributes to include "layout".
fluiddot
added
[Type] Bug
|
Size Change: 0 B Total Size: 1.13 MB ℹ️ View Unchanged
|
Additionally, the test case has been nested in a describe block.
fluiddot
commented
Jan 11, 2022
Comment on lines
+45
to
+47
| const layoutBlockSupport = getBlockSupport( name, '__experimentalLayout' ); | ||
| const defaultBlockLayout = layoutBlockSupport?.default; | ||
| const usedLayout = layout || defaultBlockLayout || {}; |
There was a problem hiding this comment.
This is based on how the layout panel fetches the layout object to be used in the inspector controls (reference).
There was a problem hiding this comment.
Thank you for providing all of these explanations! 😃
Comment on lines
+11
to
+16
| // This filter is removed because layout styles shouldn't be added | ||
| // until layout types are supported in the native version. | ||
| removeFilter( | ||
| 'editor.BlockListBlock', | ||
| 'core/editor/layout/with-layout-styles' | ||
| ); |
There was a problem hiding this comment.
Here you can see the logic related to this hook:
gutenberg/packages/block-editor/src/hooks/layout.js
Lines 183 to 226 in 29ec9e0
Comment on lines
+18
to
+23
| // This filter is removed because the layout controls shouldn't be | ||
| // enabled until layout types are supported in the native version. | ||
| removeFilter( | ||
| 'editor.BlockEdit', | ||
| 'core/editor/layout/with-inspector-controls' | ||
| ); |
There was a problem hiding this comment.
Here you can see the logic related to this hook:
gutenberg/packages/block-editor/src/hooks/layout.js
Lines 160 to 181 in 29ec9e0
There was a problem hiding this comment.
In the Buttons block, regarding the justification content controls, instead of using the one provided by the layout logic:
gutenberg/packages/block-editor/src/layouts/flex.js
Lines 69 to 86 in 29ec9e0
We define it within the block render:
gutenberg/packages/block-library/src/buttons/edit.native.js
Lines 126 to 138 in 29ec9e0
There was a problem hiding this comment.
Not suggesting a change here, but I'm wondering whether there is an opportunity to extract some of what is in the web implementation. Certainly not a blocker for this PR, just thinking about a possibility where FlexLayoutJustifyContentControl was extracted, if that would highlight where the platforms deviate. It may be that it isn't worth trying to merge components at that level, if it is mostly UI, but I'm thinking more for the sake of the logical parts of it, like the allowedControls, etc. 🤔
There was a problem hiding this comment.
Yeah though the FlexLayoutJustifyContentControl component looks very coupled to the flex layout, which we don't have implemented support in the native version. In any case, I see that the only difference between the implementation of this control between Buttons block and flex layout is the support of space-between option. Maybe if we add support for that option in the native version, we might consider enabling the layout inspector tools hook:
gutenberg/packages/block-editor/src/hooks/layout.js
Lines 238 to 242 in 43f9ad7
fluiddot
requested review from
hypest and
mkevins
and removed request for
ajitbohra
2 tasks
mkevins
approved these changes
Jan 14, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Gutenberg Mobile PR: wordpress-mobile/gutenberg-mobile#4451
Description
In #35819 the attributes
contentJustificationandorientationwere removed from the Buttons block schema and replaced by the__experimentalLayoutattribute. However, the native version of the block wasn't updated and led to the content justification alignment option stops working.gutenberg/packages/block-library/src/buttons/edit.native.js
Line 33 in 43f9ad7
As outlined in the deprecation code for the Buttons block:
gutenberg/packages/block-library/src/buttons/deprecated.js
Lines 18 to 34 in 43f9ad7
These attributes are now provided within the
layoutattribute, however, for having access to this attribute first, we need to enable the layout hooks:gutenberg/packages/block-editor/src/hooks/layout.js
Lines 228 to 242 in 43f9ad7
Specifically the hook
core/layout/addAttributewhich is in charge of adding thelayoutattribute to the block:gutenberg/packages/block-editor/src/hooks/layout.js
Lines 137 to 158 in 43f9ad7
This PR imports the layout hooks for the native version, but it only enables the required hook by removing the rest, as the native version doesn't support yet the different layout types hence, we can't include either the layout styles or layout controls.
How has this been tested?
Manual test
Integration test
Additionally, test cases have been added to verify that the content justification attribute produces the expected HTML output using snapshot testing.
If the introduced test would have been run before the #35819 changes, we would have produced the following snapshot with the old attributes:
Click here to reveal the snapshot
So if the same test is run with current changes in
trunk, it would have failed because the HTML output is different:Click here to reveal the snapshot
As a side note, the snapshot generated after applying this fix can be checked here.
Screenshots
buttons-block-justification-options.mp4
Types of changes
Bug fix
Checklist:
*.native.jsfiles for terms that need renaming or removal).