Add native version of BlockMobileToolbar (Ported from gutenberg-mobile) #16177
Conversation
Tug
added
the
Mobile App - i.e. Android or iOS
Tug
force-pushed
the
rnmobile/add/block-mobile-toolbar-component
branch
from
d4254c4 to
8f94c78
Compare
| } ) => ( | ||
| <> | ||
| <ToolbarButton | ||
| accessibilityLabel={ __( 'Move up' ) } |
There was a problem hiding this comment.
Is the accessibilityLabel needed if it's the same as label? If it is, then it's missing from the Move down button
There was a problem hiding this comment.
+1 about the accessibility labels concern.
Current develop has labels with more information, that (I think) are also used on UI Testing.
https://github.com/wordpress-mobile/gutenberg-mobile/blob/40c1374072a37b5bdf4c8730af3b68f5a5fb0f09/src/block-management/inline-toolbar/index.js#L58-L74
And the prop to set the accessibility label is actually title.
There was a problem hiding this comment.
Thanks for noticing, updating this 👍
| const normalizedClientIds = castArray( clientIds ); | ||
| const firstClientId = first( normalizedClientIds ); | ||
| const rootClientId = getBlockRootClientId( first( normalizedClientIds ) ); | ||
| const blockOrder = getBlockOrder( rootClientId ); |
There was a problem hiding this comment.
This seems to be only used for its length, maybe getBlockCount is a better selector?
On the other hand, if we're not actually looking at the results of getBlockOrder, couldn't we use the length of normalizedClientIds instead to find the last one?
There was a problem hiding this comment.
Actually, I just realized this is just what the web does. Do you think it'd make sense to share the select/dispatch logic and just fork the internal BlockMover component?
There was a problem hiding this comment.
I'm not much in favor. Surely the components have similar names but they're not used the same way at the moment in web and native, I'd rather we keep them separated.
But in general, I'm all for reusing the web code whenever possible.
| <View style={ styles.toolbar }> | ||
| <BlockMover clientIds={ [ clientId ] } /> | ||
|
|
||
| <View style={ styles.spacer } /> |
There was a problem hiding this comment.
I think that flexbox allows for this without the extra spacer view, although I haven't managed to do it directly on the inspector slot without a wrapper view:
<View style={ styles.inspectorControls }>
<InspectorControls.Slot />
</View>.inspectorControls {
flex-grow: 1;
align-items: flex-end;
}There was a problem hiding this comment.
I think it makes sense with flexbox. It's not like we're floating our elements left and right, we're pushing it to the sides thanks to a spacer.
A good alternative would be to make 2 groups of icons, each using flex as well and justifying content to the left and right. I did experiment a bit with it but I saw some strange behavior with the toolbar disappearing sometimes. The current solution seems to be working fine for us...
Tug
force-pushed
the
rnmobile/add/block-mobile-toolbar-component
branch
from
8f94c78 to
4de9db1
Compare
Tug
force-pushed
the
rnmobile/add/block-mobile-toolbar-component
branch
from
89654e4 to
8611c43
Compare
| /* translators: accessibility text. %1: current block position (number). %2: next block position (number) */ | ||
| __( 'Move block down from row %1$s to row %2$s' ), | ||
| lastIndex + 1, | ||
| lastIndex |
There was a problem hiding this comment.
On the first block, the move down button is read by VoiceOver as: "Move block down from row 1 to row 0".
It should say "... from row 1 to row 2".
The move up button is correct 👍
There was a problem hiding this comment.
Thanks, for noticing, should be fixed now :)
Description
This is step 4 of wordpress-mobile/gutenberg-mobile#958
This PR ports the gutenberg-mobile InlineToolbar to gutenberg as the BlockMobileToolbar component inside @wordpress/block-editor.
This is needed to implement Inner blocks for mobile native. You can follow the progress of the port here wordpress-mobile/gutenberg-mobile#958
How has this been tested?
wordpress-mobile/gutenberg-mobile#1133
Types of changes
Adds native support for a block editor component
Checklist: