Mobile - Block Actions Menu - Fix block title regression and adds integration tests #46699
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
… showing undefined in the Picker's header. It also adds integration tests for several actions like: Copy, Cut, Paste, Duplicate and Transform blocks
geriux
added
Mobile App - i.e. Android or iOS
|
Size Change: +58 B (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
tyxla
reviewed
Dec 21, 2022
| @@ -98,7 +98,7 @@ const BlockMobileToolbar = ( { | |||
| </BlockSettingsButton.Slot> | |||
|
|
|||
| <BlockActionsMenu | |||
| clientIds={ [ clientId ] } | |||
| clientId={ clientId } | |||
There was a problem hiding this comment.
Confirming this is the sole usage of this component.
| @@ -452,7 +448,7 @@ export default compose( | |||
| rootClientId | |||
| ); | |||
| } else { | |||
| replaceBlocks( clientIds, clipboardBlock ); | |||
| replaceBlocks( clientId, clipboardBlock ); | |||
There was a problem hiding this comment.
Even though it uses replaceBlocks under the hood, shouldn't we use replaceBlock() here for semantic accuracy?
There was a problem hiding this comment.
Good idea, I've updated it in 69e76ed
| }, | ||
| onMoveDown: ( ...args ) => | ||
| moveBlocksDown( clientIds, rootClientId, ...args ), | ||
| moveBlocksDown( clientId, rootClientId, ...args ), |
There was a problem hiding this comment.
Won't this fail when attempting to verify that blocks can be moved, because canMoveBlocks() still expects an array?
There was a problem hiding this comment.
You're right, It turns out I was testing the same functionality but from another component which was not this one 😓 I've updated the code and the test accordingly in 82c2b85
Thanks for catching that!
| normalizedClientIds[ normalizedClientIds.length - 1 ] | ||
| ); | ||
| const isFirst = firstIndex === 0; | ||
| const isLast = clientId === blockOrder.length - 1; |
There was a problem hiding this comment.
This feels off - comparing the client ID (which I expect to be a unique string) to the index of the blocks. Did we mean to use getBlockIndex() here as well?
As a matter of fact, since we no longer work with an array of client IDs, shouldn't we just be using getBlockIndex( clientId ) for the last index, just like we are already doing for the first index?
There was a problem hiding this comment.
It was indeed off 😅 thank you for spotting that, I've updated this logic in 82c2b85#diff-5977ed5d4a2d269bae979099b537fe8daef530d2513e09ae1fedabb374e82c52
added 6 commits
December 21, 2022 15:32
…ast index. It also passes the clientId as an array for the moveBlocksUp and moveBlocksDown
geriux
commented
Dec 21, 2022
| @@ -67,6 +67,7 @@ describe( 'Block Actions Menu', () => { | |||
| it( 'moves blocks up and down', async () => { | |||
| const screen = await initializeEditor( { | |||
| initialHtml: INITIAL_HTML, | |||
| screenWidth: 100, | |||
There was a problem hiding this comment.
This makes the editor canvas to be smaller so the move-up/down options show in the block actions menu instead of the usual up/down arrows below the blocks, this way we can test that functionality.
In this example, I forced the editor to show up like that so I could show those options, it is not a real example of when the up/down buttons are collapsed.
tyxla
approved these changes
Dec 21, 2022
There was a problem hiding this comment.
The code looks great, thanks for fixing the regression and adding great tests @geriux 🙌 💯
I'd prefer deferring testing to someone who has the capability to do it - I'm on a different machine and have been procrastinating the mobile setup for a while 😅
| const normalizedClientIds = Array.isArray( clientIds ) | ||
| ? clientIds | ||
| : [ clientIds ]; | ||
| const block = getBlock( normalizedClientIds ); |
There was a problem hiding this comment.
Wow, how was this working with an array of client IDs 😅 Looking at getBlock() it seems to only a single client ID, and not array of one. Glad to see that fixed 👍
There was a problem hiding this comment.
I know 😄 haha that's why I added a few tests to make sure we know if it breaks in the future.
| ? clientIds | ||
| : [ clientIds ]; | ||
| const block = getBlock( normalizedClientIds ); | ||
| const blockName = getBlockName( normalizedClientIds ); |
There was a problem hiding this comment.
Same as for getBlock() - getBlockName() only seems to work with a single block client ID, so nice to see that fixed as well.
Comment on lines
+335
to
+337
| const currentBlockIndex = getBlockIndex( clientId ); | ||
| const isFirst = currentBlockIndex === 0; | ||
| const isLast = currentBlockIndex === blockOrder.length - 1; |
| @@ -426,12 +421,12 @@ export default compose( | |||
| ); | |||
| }, | |||
| duplicateBlock() { | |||
| return duplicateBlocks( clientIds ); | |||
| return duplicateBlocks( [ clientId ] ); | |||
There was a problem hiding this comment.
FWIW, this should also work with a single value since getBlocksByClientId() is used inside of duplicateBlocks, and that performs a cast to an array.
However, in favor of better use of the API, I'm happy to keep the specific array here, given that the function is in plural.
| onMoveUp: ( ...args ) => | ||
| moveBlocksUp( clientIds, rootClientId, ...args ), | ||
| moveBlocksUp( [ clientId ], rootClientId, ...args ), |
There was a problem hiding this comment.
A side note (nothing we need to address in this PR): seems like createOnMove() no longer needs to cast the client IDs to an array, since it already expects only an array.
| upButton.props.accessibilityState?.disabled; | ||
| expect( isUpButtonDisabled ).toBe( true ); | ||
|
|
||
| // Press the button to make sure the move doesn't block |
There was a problem hiding this comment.
Nit: Perhaps we misplaced some words:
Suggested change
| // Press the button to make sure the move doesn't block | |
| // Press the button to make sure the button doesn't move |
| downButton.props.accessibilityState?.disabled; | ||
| expect( isDownButtonDisabled ).toBe( true ); | ||
|
|
||
| // Press the button to make sure the move doesn't block |
There was a problem hiding this comment.
Suggested change
| // Press the button to make sure the move doesn't block | |
| // Press the button to make sure the block doesn't move |
Comment on lines
+143
to
+145
| const isDownButtonDisabled = | ||
| downButton.props.accessibilityState?.disabled; | ||
| expect( isDownButtonDisabled ).toBe( true ); |
There was a problem hiding this comment.
Would be nice if we had .toBeDisabled(). It exists in jest-native, which we might want to use eventually:
https://github.com/testing-library/jest-native#tobedisabled
There was a problem hiding this comment.
For this case, we are not using a built-in Button but custom components so toBeDisabled is not available.
| upButton.props.accessibilityState?.disabled; | ||
| expect( isUpButtonDisabled ).toBe( true ); | ||
|
|
||
| // Press the button to make sure the move doesn't block |
There was a problem hiding this comment.
Suggested change
| // Press the button to make sure the move doesn't block | |
| // Press the button to make sure the block doesn't move |
| downButton.props.accessibilityState?.disabled; | ||
| expect( isDownButtonDisabled ).toBe( true ); | ||
|
|
||
| // Press the button to make sure the move doesn't block |
There was a problem hiding this comment.
Suggested change
| // Press the button to make sure the move doesn't block | |
| // Press the button to make sure the block doesn't move |
added 3 commits
December 21, 2022 18:06
…thin the modal, since they're mutltiple elements with the Move up/down labels
…ad of using pre-defined content
Thank you so much for reviewing @tyxla!
No worries! @derekblank could you check this PR and test it whenever you have a chance, please? Thank you! |
derekblank
approved these changes
Dec 22, 2022
geriux
added
Mobile App - i.e. Android or iOS
geriux
pushed a commit
that referenced
this pull request
Dec 27, 2022
…egration tests (#46699) * Mobile - Block Actions Menu - Fix regression where the block name was showing undefined in the Picker's header. It also adds integration tests for several actions like: Copy, Cut, Paste, Duplicate and Transform blocks * Mobile - Tests - Initialize Editor helper - Add option to specify the screen width * Mobile - Style mocks - Pass the marginLeft value for the Default block styles * Mobile - Block Mover - Add tests for moving up and down blocks * Mobile - Block Actions Menu - Fix logic for selecting the first and last index. It also passes the clientId as an array for the moveBlocksUp and moveBlocksDown * Mobile - Block Actions Menu - Use replaceBlock instead of replaceBlocks * Mobile - Block Actions test and Block Mover test - Fix typos * Mobile - Block Actions Menu Test - Add testID to match for options within the modal, since they're mutltiple elements with the Move up/down labels * Mobile - Block Mover tests - Update test to add blocks manually instead of using pre-defined content
4 tasks
geriux
pushed a commit
that referenced
this pull request
Dec 27, 2022
* Release script: Update react-native-editor version to 1.86.0 * Release script: Update with changes from 'npm run core preios' * Release script: Update react-native-editor version to 1.86.1 * Release script: Update with changes from 'npm run core preios' * Mobile - Block Actions Menu - Fix block title regression and adds integration tests (#46699) * Mobile - Block Actions Menu - Fix regression where the block name was showing undefined in the Picker's header. It also adds integration tests for several actions like: Copy, Cut, Paste, Duplicate and Transform blocks * Mobile - Tests - Initialize Editor helper - Add option to specify the screen width * Mobile - Style mocks - Pass the marginLeft value for the Default block styles * Mobile - Block Mover - Add tests for moving up and down blocks * Mobile - Block Actions Menu - Fix logic for selecting the first and last index. It also passes the clientId as an array for the moveBlocksUp and moveBlocksDown * Mobile - Block Actions Menu - Use replaceBlock instead of replaceBlocks * Mobile - Block Actions test and Block Mover test - Fix typos * Mobile - Block Actions Menu Test - Add testID to match for options within the modal, since they're mutltiple elements with the Move up/down labels * Mobile - Block Mover tests - Update test to add blocks manually instead of using pre-defined content * Mobile - Update Changelog * Mobile - Update Podfile Co-authored-by: Derek Blank <derekpblank@gmail.com>
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.
Fixes #46673
Fixes wordpress-mobile/gutenberg-mobile#5337
Related PRs:
What?
After some changes were introduced in #43849 related to the refactor of the
lodashlibrary effort, the block title within the block actions menu picker was showing asundefined.Why?
To fix a regression in the mobile editor related to the block actions menu.
How?
By simplifying the code by passing just the current
cliendIdas a string instead of an array with just oneclientId, by updating this and adapting the code, it now gets the right block title in the block actions menu.It also adds several integration tests:
Block Actions Menu
✓ renders the block name in the Picker's header
moving blocks (from the block actions menu, these show up when the canvas is smaller so we hide the up/down arrows below blocks and show them in the actions menu instead)
✓ moves blocks up and down
✓ disables the Move Up button for the first block
✓ disables the Move Down button for the last block
block options
✓ copies and pastes a block
✓ does not replace a non empty Paragraph block when pasting another block
✓ cuts and pastes a block
✓ duplicates a block
✓ transforms a Paragraph block into a Pullquote block
Block Mover Picker
moving blocks (The up/down arrows that show up below a block)
✓ moves blocks up and down
✓ disables the Move Up button for the first block
✓ disables the Move Down button for the last block
Testing Instructions
Even though there are new integration tests that should cover quite a few cases, let's smoke-test using the demo app as well.
...three dots button to open the block actions menuTesting Instructions for Keyboard
N/A
Screenshots or screencast