[RNMobile] Do not include MediaReplaceFlow component in native Gallery toolbar #52966
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
derekblank
added
Mobile App - i.e. Android or iOS
|
Size Change: +366 B (0%) Total Size: 1.44 MB
ℹ️ View Unchanged
|
dcalhoun
approved these changes
Jul 26, 2023
There was a problem hiding this comment.
Thanks for addressing this so quickly! 👏🏻
3. The native Gallery tests are updated to check that the MediaReplaceFlow component is not present in the toolbar. This seemed like the most direct way to test that the visual gap would not be present within an integration test, but I am open to other ideas that may be more elegant.
I agree. This seems like the most direct approach for asserting this outcome. My only other suggestion might be that we could forgo an automated test as the bug isn't all that disruptive or critical. I do not have a strong opinion on this particular matter.
| numberOfItems: 3, | ||
| media, | ||
| } ); | ||
| const { queryByTestId } = screen; |
There was a problem hiding this comment.
I recommend avoiding destructuring the result of render (i.e. initializeWithGalleryBlock) as destructuring requires updating the destructured values whenever adding or changing queries.
Unrelated to this specific change: when adding new test files or refactoring existing tests, I recommend leveraging the screen import as well. There is little usage in the project so far. My practice is to only use it in new test files as I didn't want to create confusion by intermingling its usage in existing tests.
There was a problem hiding this comment.
Unrelated to this specific change: when adding new test files or refactoring existing tests, I recommend leveraging the
screenimport as well. There is little usage in the project so far. My practice is to only use it in new test files as I didn't want to create confusion by intermingling its usage in existing tests.
That's interesting. I saw it was introduced in version 10.1.0 of the RN testing library, that explains why we didn't use it when we started to implement integration tests.
In relation to this, at some point, we should update the integration test guide to reflect this.
packages/block-editor/src/components/media-replace-flow/index.native.js
Outdated
Show resolved
Hide resolved
Co-authored-by: David Calhoun <github@davidcalhoun.me>
This was referenced Jul 27, 2023
4 tasks
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.
What?
Resolves an issue with removing an extra visual gap in the mobile toolbar.
Related:
Why?
Under the new Editor UX changes (released in Gutenberg 1.100.1 / WordPress 22.9), the mobile block toolbar buttons were rearranged, with the Block "More" and "Settings" buttons being moved from the block to the toolbar itself. In the native Gallery block, this change exposed an extra BlockControls component gap within the toolbar wrapping the MediaReplaceFlow component:
gutenberg/packages/block-library/src/gallery/edit.js
Lines 638 to 651 in bdde553
MediaReplaceFlow is not yet implemented on mobile, and the native component currently returns an empty null component value. As the Gallery block shares the edit.js file between web and mobile, this extra BlockControls separator is wrapping an empty component. The addition of moving the More and Settings buttons after this within the toolbar adds a unnecessary double border from the BlockControls component.
How?
Platform.isWebboolean block so it is not present in the mobile toolbar.<View />with a testID so that it can be tested.Testing Instructions
Screenshots or screencast