Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RNMobile] Do not include MediaReplaceFlow component in native Gallery toolbar #52966

Merged
merged 4 commits into from
Jul 26, 2023
Merged

[RNMobile] Do not include MediaReplaceFlow component in native Gallery toolbar #52966

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
63000fe
Do not display MediaReplaceFlow component in native Gallery block
derekblank Jul 26, 2023
9439a31
Add integration test to hide MediaReplaceFlow component in Gallery block
derekblank Jul 26, 2023
0b0532a
Update CHANGELOG
derekblank Jul 26, 2023
54df57d
Update test selector syntax
derekblank Jul 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions packages/block-editor/src/components/media-replace-flow/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,12 @@
// MediaReplaceFlow component is not yet implemented in the native version,
// so we return an empty component instead.
export default () => null;
/**
* External dependencies
*/
import { View } from 'react-native';

// MediaReplaceFlow component is not yet implemented in the native version.
// For testing purposes, we are using an empty View component with a testID prop.
const MediaReplaceFlow = () => {
return <View testID="media-replace-flow" />;
};

export default MediaReplaceFlow;
38 changes: 20 additions & 18 deletions packages/block-library/src/gallery/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -635,25 +635,27 @@ function GalleryEdit( props ) {
/>
) }
</BlockControls>
<BlockControls group="other">
<MediaReplaceFlow
allowedTypes={ ALLOWED_MEDIA_TYPES }
accept="image/*"
handleUpload={ false }
onSelect={ updateImages }
name={ __( 'Add' ) }
multiple={ true }
mediaIds={ images
.filter( ( image ) => image.id )
.map( ( image ) => image.id ) }
addToGallery={ hasImageIds }
/>
</BlockControls>
{ Platform.isWeb && (
<GapStyles
blockGap={ attributes.style?.spacing?.blockGap }
clientId={ clientId }
/>
<>
<BlockControls group="other">
<MediaReplaceFlow
allowedTypes={ ALLOWED_MEDIA_TYPES }
accept="image/*"
handleUpload={ false }
onSelect={ updateImages }
name={ __( 'Add' ) }
multiple={ true }
mediaIds={ images
.filter( ( image ) => image.id )
.map( ( image ) => image.id ) }
addToGallery={ hasImageIds }
/>
</BlockControls>
<GapStyles
blockGap={ attributes.style?.spacing?.blockGap }
clientId={ clientId }
/>
</>
) }
<Gallery
{ ...props }
Expand Down
14 changes: 14 additions & 0 deletions packages/block-library/src/gallery/test/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -622,6 +622,20 @@ describe( 'Gallery block', () => {
expect( getEditorHtml() ).toMatchSnapshot();
} );

it( 'does not display MediaReplaceFlow component within the block toolbar', async () => {
const screen = await initializeWithGalleryBlock( {
numberOfItems: 3,
media,
} );
const { queryByTestId } = screen;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.


fireEvent.press( getBlock( screen, 'Gallery' ) );

// Expect the native MediaReplaceFlow component to not be present in the block toolbar
const mediaReplaceFlow = queryByTestId( 'media-replace-flow' );
expect( mediaReplaceFlow ).toBeNull();
} );

// Test cases related to TC013 - Settings - Columns
// Reference: https://github.com/wordpress-mobile/test-cases/blob/trunk/test-cases/gutenberg/gallery.md#tc013
describe( 'Columns setting', () => {
Expand Down
1 change: 1 addition & 0 deletions packages/react-native-editor/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ For each user feature we should also add a importance categorization label to i
-->

## Unreleased
- [*] Remove visual gap in mobile toolbar when a Gallery block is selected [#52966]

## 1.100.1
- [**] Add WP hook for registering non-core blocks [#52791]
Expand Down