[RNMobile] Simplify media insertion flow - Part 3 component integration #29548
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
jd-alexander
added
the
Mobile App - i.e. Android or iOS
jd-alexander
commented
Mar 4, 2021
Comment on lines
+471
to
+486
| withDispatch( ( dispatch, { clientId }, { select } ) => { | ||
| return { | ||
| wasBlockJustInserted() { | ||
| const { clearLastBlockInserted } = dispatch( 'core/editor' ); | ||
| const { wasBlockJustInserted } = select( 'core/editor' ); | ||
|
|
||
| const result = wasBlockJustInserted( clientId ); | ||
|
|
||
| if ( result ) { | ||
| clearLastBlockInserted(); | ||
| return true; | ||
| } | ||
| return false; | ||
| }, | ||
| }; | ||
| } ), |
There was a problem hiding this comment.
I repeated this withDispatch logic in three places. I am not sure how to make this reusable hence the duplication. Instinctively, I thought of hooks but these are class components so they are incompatible and the nature of this PR is not to do this conversion for these components. Let me know if you have any thoughts on this.
There was a problem hiding this comment.
I also wondered if this logic could somehow live within the store, but I wasn't sure about this either.
There was a problem hiding this comment.
I agree finding a way to reuse this would be a nice plus. I'd need to take time to play around with it to make a suggestion though as I've been out of the RN side for a little while now.
jd-alexander
commented
Mar 4, 2021
| return { | ||
| wasBlockJustInserted() { | ||
| const { clearLastBlockInserted } = dispatch( 'core/editor' ); | ||
| const { wasBlockJustInserted } = select( 'core/editor' ); |
There was a problem hiding this comment.
I am getting a warning about using the string literals to access the data store. I haven't seen a store definition for core-editor in other parts of the codebase as yet.
There was a problem hiding this comment.
I just found the PR that replaced store names and I saw that the core/editor is now editorStore
There was a problem hiding this comment.
I have the changes for this locally. I am encountering the error below. I will look into what could be causing in the morning
There was a problem hiding this comment.
I didn't see this error pop up for me so you probably have it fixed now. This sounds similar to an issue solved in this PR where a setting was removed that the mobile side counted on.
|
Size Change: +89 B (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
7 tasks
jd-alexander
commented
Mar 4, 2021
| @@ -6,6 +6,9 @@ import { blockNames } from './pages/editor-page'; | |||
| describe( 'Gutenberg Editor Gallery Block tests', () => { | |||
| it( 'should be able to add a gallery block', async () => { | |||
| await editorPage.addNewBlock( blockNames.gallery ); | |||
| await editorPage.driver.sleep( 1000 ); | |||
There was a problem hiding this comment.
This delay and the one below ensures the picker has been opened before attempting to close it.
7 tasks
…to rnmobile/simplify_image_insertion_flow-component-integration
chipsnyder
reviewed
Mar 8, 2021
There was a problem hiding this comment.
Nice work @jd-alexander! Tested this out on iOS and Android using the demo apps and everything works as expected. I agree with your comment to reuse the withDispatch changes but I've been away from RN so I didn't have a good suggestion there yet.
One other thought, I wonder if we should auto-open on File and Audio as well. WDYT?
Comment on lines
+471
to
+486
| withDispatch( ( dispatch, { clientId }, { select } ) => { | ||
| return { | ||
| wasBlockJustInserted() { | ||
| const { clearLastBlockInserted } = dispatch( 'core/editor' ); | ||
| const { wasBlockJustInserted } = select( 'core/editor' ); | ||
|
|
||
| const result = wasBlockJustInserted( clientId ); | ||
|
|
||
| if ( result ) { | ||
| clearLastBlockInserted(); | ||
| return true; | ||
| } | ||
| return false; | ||
| }, | ||
| }; | ||
| } ), |
There was a problem hiding this comment.
I agree finding a way to reuse this would be a nice plus. I'd need to take time to play around with it to make a suggestion though as I've been out of the RN side for a little while now.
Thanks for the review 🙇🏾
Good point, I think it would make sense to open them as well. I am going to create another PR with that implementation once these have been finalized and merged in. |
chipsnyder
approved these changes
Mar 9, 2021
guarani
approved these changes
Mar 9, 2021
…to rnmobile/simplify_image_insertion_flow-component-integration
Thanks much for all the reviews and comments @guarani 🙇🏾 . I have synced all branches with trunk and I will be resolving the test issues and merging. A label in the e2e test says "Dismiss" instead of "Cancel" so I will update that. |
jd-alexander
merged commit 232a0cd
into
rnmobile/simplify_image_insertion_flow-media-upload
jd-alexander
deleted the
rnmobile/simplify_image_insertion_flow-component-integration
branch
jd-alexander
added a commit
that referenced
this pull request
Mar 16, 2021
* added autoOpenMediaUpload prop to the MediaPlaceholder * Added the auto-opening capabilities to the MediaUpload component. * Added documentation for the new autoOpenMediaUpload prop * renamed autoOpenMediaUpload to autoOpen in the MediaUpload component. * [RNMobile] Simplify media insertion flow - Part 3 component integration (#29548) * Track the clientId of the block that is inserted. * implemented auto opening utilizing last block inserted from the store * added dismissal support for the auto opening picker to the UI tests. * Updated Dismiss button in closePicker function to Cancel
jd-alexander
added a commit
that referenced
this pull request
Apr 20, 2021
* created actions for adding and clearing last inserted block event. * added reducer for determining new state based on the action * added selector to query the state for the last block inserted * [RNMobile] Simplify media insertion flow Part 2 - media upload (#29547) * added autoOpenMediaUpload prop to the MediaPlaceholder * Added the auto-opening capabilities to the MediaUpload component. * Added documentation for the new autoOpenMediaUpload prop * renamed autoOpenMediaUpload to autoOpen in the MediaUpload component. * [RNMobile] Simplify media insertion flow - Part 3 component integration (#29548) * Track the clientId of the block that is inserted. * implemented auto opening utilizing last block inserted from the store * added dismissal support for the auto opening picker to the UI tests. * Updated Dismiss button in closePicker function to Cancel * Added release notes for auto-opening. * [RNMobile] Refactor simplify media flow redux store changes (#30123) * Moved the last block inserted actions from editor to the block-editor * Moved the last block inserted reducer from editor to the block-editor * Moved the last block inserted selector from editor to the block-editor * Fixed es-lint error. * Moved last block inserted actions test from editor to the block-editor * Moved last block inserted reducer test from editor to the block-editor * Moved last block inserted selector test from editor to the block-editor * Moved all calls to last block inserted from editor to block-editor * last block inserter usage in menu native migrated : editor to block-editor * [RNMobile] Refactor: Simplify media flow redux migration (#30238) * Add meta argument to insertBlock action * Add inserter menu source * Update last block inserted reducer Instead of using specific actions for tracking the last block inserted, it uses the actions related to the insertion flow. * Add get last block inserted selector * Refactor gallery edit component withSelect and withDispatch logic has been refactored to use useSelect and useDispatch hooks * Refactor image edit component wasBlockJustInserted is now calculated with getLastBlockInserted * Refactor video edit component wasBlockJustInserted is now calculated with getLastBlockInserted * Fix reset blocks action in last block inserted reducer * Add source param to wasBlockJustInserted selector * Simplify withSelect part of video block * Removed add/clear last block inserted actions and tests due to refactor * Removed addLastBlockInserted from the insert menu's onPress. * rewrote the tests for the lastBlockInserted reducer. * rewrote tests for wasBlockJustInserted selector. * optimized clientId * removed unneeded clientId. * put the expectedSource inside the test meta object. * used expectedState insted {} for state related tests. * used expectedState instead {} for state related tests. * removed parentheses from describe name. * return the same state instead of empty state. * made changes to test name so its intent is clearer. * made the insertion source optional. Co-authored-by: Carlos Garcia <fluiddot@gmail.com> * added wasBlockJustInserted prop needed after merge with trunk. * removed updateSelection from reducer so it's updated at all times. Co-authored-by: Carlos Garcia <fluiddot@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.
gutenberg-mobilePR wordpress-mobile/gutenberg-mobile#2700Builds on top of #29547 - Do not merge until this PR has been merged and the base branch is updated to
trunk.Description
This change simplifies the media insertion flow by showing the media options sheet when you tap
Image,VideoorGalleryto add the media placeholder to the canvas. Before you would have to add the media placeholder and clickAdd Image / Video / Mediafor it to be shown after insertion.Testing
Image
Video
Gallery
Regression Testing
Checklist: