[RNMobile] Gallery block: Fix crash when adding images and selecting a gallery item #38238
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
Since the MediaReplaceFlow component is not implemented yet in the native version of the editor, we return an empty component instead.
fluiddot
added
[Type] Bug
|
Size Change: +46 B (0%) Total Size: 1.14 MB
ℹ️ View Unchanged
|
`autoOpenMediaUpload` prop is only used in the native version of the `MediaPlaceholder` component hence, we don't need to define it in the props set for the web version.
Thank you @geriux for reviewing the PR ❤️ ! I've just reviewed and applied the suggestions you mentioned, let me know if you could take another review, thanks 🙇. |
geriux
approved these changes
Jan 26, 2022
|
This tested well for me on the web side and the code LGTM. What is the best way for us to minimise the chance of this sort of surprise on the mobile side in the future - should we ping some mobile team members whenever we change the gallery? |
Thanks for taking a look @glendaviesnz, I really appreciate it 🙇.
Ideally, these issues should be identified by automatic tests (integration + e2e tests). In this case, as there was none for the Gallery block, we spotted it probably too late during the manual testing. Nevertheless, currently, we're focused on increasing the coverage of blocks, especially for preventing cases like this, so hopefully, that should help us in the future. Regarding pinging, I see that for the Gallery block @mkevins is assigned as a code owner (reference), but not sure if Matthew was able to take a look. It would be great to figure out better ways to sync changes between both platforms, but in the meantime, I think pinging someone from the mobile team is a good workaround 👍 . |
fluiddot
added a commit
that referenced
this pull request
Jan 27, 2022
…a gallery item (#38238) * Add the native variant of MediaReplaceFlow Since the MediaReplaceFlow component is not implemented yet in the native version of the editor, we return an empty component instead. * Set MediaPlaceholder props by platform in gallery * Update react-native-editor changelog * Remove autoOpenMediaUpload from web props `autoOpenMediaUpload` prop is only used in the native version of the `MediaPlaceholder` component hence, we don't need to define it in the props set for the web version. * Add gallery block integration tests * Unify shared props between web and native
I did get a notification for this, but I think I overlooked it as it was purple by the time I saw it 😞 .
This is definitely the way to go, imo. Also, as we continue to improve the developer experience of building and running the mobile app, direct manual testing of the areas related to a PR's changes can be a great holistic way to cover these kinds of issues, and will likely help to foster the "mobile caution/awareness" upstream. I.e. the tighter that loop is, the better. |
It looks like the gallery block on-device test is still present, but it merely adds an empty gallery block, which will only catch a limited subset of potential issues. That suite could certainly use some expansion 😄 |
Certainly, we could also add integration tests to cover the block's logic and user flows. |
This would be fantastic! Especially now that the gallery block is in a more "stable" state from a UI perspective (i.e. using inner blocks). One of the blockers to adding such tests earlier was that the block was undergoing big changes to the UI with the format refactoring, and now that this phase is complete (more or less), integration / UI tests will help catch (and prevent) a whole class of issues before they even land. |
@mkevins Great, thanks for sharing the context 🙇 . FYI I planned to tackle integration tests for this block, I'll let you know once I finish the PRs in case you could take a look at the user flows covered, thanks. |
dcalhoun
added a commit
that referenced
this pull request
Feb 2, 2022
* Release script: Update react-native-editor version to 1.70.0 * Release script: Update with changes from 'npm run core preios' * Add 1.70.0 section to react-native-editor changelog * Release script: Update react-native-editor version to 1.70.1 * Release script: Update with changes from 'npm run core preios' * [RNMobile] Gallery block: Fix crash when adding images and selecting a gallery item (#38238) * Add the native variant of MediaReplaceFlow Since the MediaReplaceFlow component is not implemented yet in the native version of the editor, we return an empty component instead. * Set MediaPlaceholder props by platform in gallery * Update react-native-editor changelog * Remove autoOpenMediaUpload from web props `autoOpenMediaUpload` prop is only used in the native version of the `MediaPlaceholder` component hence, we don't need to define it in the props set for the web version. * Add gallery block integration tests * Unify shared props between web and native * Add 1.70.1 section to react-native-editor changelog * Declare package visibility to query URL handler apps (#38377) * Declare package visibility to query URL handler apps * Update changelog * Update react-native-bridge changelog Co-authored-by: Stefanos Togoulidis <stefanostogoulidis@gmail.com> Co-authored-by: David Calhoun <438664+dcalhoun@users.noreply.github.com>
geriux
pushed a commit
that referenced
this pull request
Feb 3, 2022
* Release script: Update react-native-editor version to 1.70.0 * Release script: Update with changes from 'npm run core preios' * Add 1.70.0 section to react-native-editor changelog * Release script: Update react-native-editor version to 1.70.1 * Release script: Update with changes from 'npm run core preios' * [RNMobile] Gallery block: Fix crash when adding images and selecting a gallery item (#38238) * Add the native variant of MediaReplaceFlow Since the MediaReplaceFlow component is not implemented yet in the native version of the editor, we return an empty component instead. * Set MediaPlaceholder props by platform in gallery * Update react-native-editor changelog * Remove autoOpenMediaUpload from web props `autoOpenMediaUpload` prop is only used in the native version of the `MediaPlaceholder` component hence, we don't need to define it in the props set for the web version. * Add gallery block integration tests * Unify shared props between web and native * Add 1.70.1 section to react-native-editor changelog * Declare package visibility to query URL handler apps (#38377) * Declare package visibility to query URL handler apps * Update changelog * Update react-native-bridge changelog * Release script: Update react-native-editor version to 1.70.2 * Release script: Update with changes from 'npm run core preios' * Mobile - Rich Text - Validate link colors * Mobile - Rich Text - Update naming to match prop * Update Changelog * GutenbergDemo - Restore unwanted archs change * Mobile - Update changelog Co-authored-by: Carlos Garcia <fluiddot@gmail.com> Co-authored-by: Stefanos Togoulidis <stefanostogoulidis@gmail.com>
fluiddot
added a commit
that referenced
this pull request
Feb 10, 2022
* Release script: Update react-native-editor version to 1.70.0 * Release script: Update with changes from 'npm run core preios' * Add 1.70.0 section to react-native-editor changelog * Release script: Update react-native-editor version to 1.70.1 * Release script: Update with changes from 'npm run core preios' * [RNMobile] Gallery block: Fix crash when adding images and selecting a gallery item (#38238) * Add the native variant of MediaReplaceFlow Since the MediaReplaceFlow component is not implemented yet in the native version of the editor, we return an empty component instead. * Set MediaPlaceholder props by platform in gallery * Update react-native-editor changelog * Remove autoOpenMediaUpload from web props `autoOpenMediaUpload` prop is only used in the native version of the `MediaPlaceholder` component hence, we don't need to define it in the props set for the web version. * Add gallery block integration tests * Unify shared props between web and native * Add 1.70.1 section to react-native-editor changelog * Declare package visibility to query URL handler apps (#38377) * Declare package visibility to query URL handler apps * Update changelog * Update react-native-bridge changelog * Release script: Update react-native-editor version to 1.70.2 * Release script: Update with changes from 'npm run core preios' * Mobile - Rich Text - Validate link colors * Mobile - Rich Text - Update naming to match prop * Update Changelog * GutenbergDemo - Restore unwanted archs change * Release script: Update react-native-editor version to 1.70.3 * Release script: Update with changes from 'npm run core preios' * [RNMobile] Highlight text: Check if style attribute value is defined during filtering (#38670) * Check if style value is defined before removing extra spaces * Add test case for old text color format * Update react-native-editor changelog Co-authored-by: Carlos Garcia <fluiddot@gmail.com> Co-authored-by: Stefanos Togoulidis <stefanostogoulidis@gmail.com> Co-authored-by: Gerardo <gerardo.pacheco@automattic.com>
8 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.
Gutenberg Mobile PR: wordpress-mobile/gutenberg-mobile#4515
Description
Fixes #38217.
After the changes introduced in #38036, we spotted in the native version that the Gallery block is trying to render the component
MediaReplaceFlowwhich is neither defined nor exported in native hence, this produces a crash in the editor.For this reason, this PR introduces the following two changes:
Create the native variant of the component
MediaReplaceFlow:As this component is not required yet by the native version, an empty component is provided for now.
Create a platform-specific set of props to be included when rendering the
MediaPlaceholdercomponent:This is required in order to keep the same behavior for the native version of the gallery block, after the changes introduced in Gallery block: Move add/edit media to block toolbar #38036.
Testing Instructions
Screenshots
N/A
Types of changes
Bug fix
Checklist:
*.native.jsfiles for terms that need renaming or removal).