Fix native BlockAlignmentControl #35191
Conversation
|
Size Change: +4.79 kB (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
dcalhoun
added
[Type] Regression
A recent web-centric change introduced usage of `MenuGroup` and `MenuItem`, both of which are unsupported in the native editor currently. This updates the conditional within `BlockAlignmentControl` to reinstate the previous UI for the native platform.
dcalhoun
force-pushed
the
bug/fix-native-block-alignment-control
branch
from
b980ef9 to
e5cb098
Compare
There was a problem hiding this comment.
I verified that the issue is no longer happening with these changes 🎊 , awesome job @dcalhoun 💯 .
The only thing I'd like to double-check before approving the PR is if the none alignment value is being properly handled in the Image block. I noticed that the none/default alignment value for that block aligns the content to the left (see attached video):
alignment-values-image.mp4
Tested on Simulator iPhone 12 Pro Max (iOS 14.5).
packages/block-editor/src/components/block-alignment-control/test/index.native.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-alignment-control/test/index.native.js
Outdated
Show resolved
Hide resolved
Rename mistakenly named assignments, which were misleading. Additionally, the reliance upon the default `value="left"` could lead to a false-positive test. Changing to a none-default of `value="right"` improves the validity of the test.
@fluiddot this is a fantastic question. Thanks for bringing this up. In regards to explicit code to "handle" a gutenberg/packages/block-library/src/image/edit.native.js Lines 623 to 629 in bcd16d6 However, I would pose the question: what should the representation be for the From reviewing the web representation, it might imply that representing Web Editor (top) vs Web Preview (bottom)
Given that, I would say our two options are presenting Native Editor `none` Presentation
|
There was a problem hiding this comment.
Thanks for handling this @dcalhoun ! 💯
I can see that the main change is extraProps = isToolbar || Platform.isNative... so the web version is not affected at all.
Just to learn a bit more, can you please share why MenuGroup and MenuItem are not supported in native? Is there any technical limitation with native?
Thanks for checking this ❤️ !
Great investigation 💯 ! I agree that only center and left alignments fit what I would expect when having As a side note, while testing I spotted that the theme might affect the Image block as well, as an example, I used the theme
|
packages/block-editor/src/components/block-alignment-control/test/index.native.js
Outdated
Show resolved
Hide resolved
|
Also curious about |
Co-authored-by: Carlos Garcia <fluiddot@gmail.com>
@ntsekouras @mtias in regards to lack of Additionally, the mobile UI often leverages more platform-specific UI like our Cell and Action Sheets
In regards to a timeline for support, I would defer to @hypest, @mchowning, @antonis, and @iamthomasbishop to ask if they have any knowledge of plans to leverage a I interpret the inquiries regarding |
|
It's not blocking now, but I want to stay as far away as we can from these platform divergences on shared components, because we are shipping extra code for anyone and just makes maintenance harder. |
|
👋 thanks for the ping @dcalhoun! There are no current plans for supporting
I agree with @mtias , it's unfortunate to have to ship and maintain platform specific "forks", especially when they end up being divergent designs. I also understand how they come to be some times: a UI change on the web is merged without being tested on native mobile (at least for crashes) which puts the native mobile team in panic mode to fix the crash in any fast/dirty way they can. It'd be better if we get to collaborate before such merges happen, to at least test out native mobile when new UI is introduced (the "none" alignment option in this case). |
|
@hypest I think we need more participation on the UI components side of things as well, since these are primitives that people making updates to blocks should not have to account for because they should have a representation cross-platform. |
Related PRs
Description
Fixes #35173. Relates to #34710. A recent web-centric change introduced usage of
MenuGroupandMenuItem, both of which are unsupported in the native editor currently. This updates the conditional withinBlockAlignmentControlto reinstate the previous UI for the native platform.How has this been tested?
Native Alignment
Web Alignment
Complete Testing Instructions found in #34710.
Screenshots
n/a
Types of changes
Bug fix
Checklist:
*.native.jsfiles for terms that need renaming or removal).