[RNMobile] Refactor simplify media flow redux store changes #30123
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
…nto rnmobile/simplify-media-flow-redux-migration
jd-alexander
commented
Mar 23, 2021
|
Size Change: +68 B (0%) Total Size: 1.4 MB
ℹ️ View Unchanged
|
I think the error is not really caused by the changes of this PR but uncovered an issue that was introduced in an older PR. As you can see in this line, we're using You can verify that the error goes away if you remove the How to fix itFollowing your comment, the best idea in my opinion would be to refactor the I did a quick modification and I think this solution should work, you can check the code diff below and if you agree I can push these changes to this branch. Patch diffThese changes shouldn't revert the performance improvement that was done in the PR because for calculating the |
|
Thanks for the review @fluiddot 🙇🏾
Indeed! I saw that error as well. Happy to see that you narrowed it to down to the
Sounds good!
Thanks! Yes, I reviewed the diff and I can see clearly where the code is migrated from the high order components to hooks. The only logic missing from the diff is that of
Makes sense. I think this migration is pretty straightforward 👍🏾 A sanity test of the Gallery block will be done here to verify all is well once the changes are settled. |
Oh I didn't realise about the undo/redo operations, actually I removed the Regarding this part, I'm thinking if there would be a better approach. I wouldn't expect that an edit component clears the value of the last block inserted when the bottom sheet is displayed. We might want that value in the future for a different logic that is executed later on, but in this case, it might be cleared before fetching it. I'm wondering if it would be possible to pass a prop with the insertion source ( |
Sounds like a good idea. I am wondering though if this solution would prevent re-renders of the auto opening. i.e if the insertion source prop gets passed to the component and we check if a block was last inserted, what happens when there are re-renders? how would we track that the bottom sheet is already shown? Currently clearing it does that but I agree with the fact that if the last block inserted functionality was needed elsewhere clearing it would break that functionality. |
7 tasks
As far as I checked re-renders are not a problem unless
I think we don't really need to track this, in my opinion the part that we have to control is the
I think clearing the last block inserted makes sense but we should trigger it in a different moment. I explored the option of passing a source when a block is inserted and I think it works fine, you can check the changes in this PR. Let me know your feedback, if you think the changes are ok, we could apply it to this PR. |
* 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
This was referenced Mar 26, 2021
…ux-migration' into rnmobile/simplify-media-flow-redux-migration
|
Hi @fluiddot this is ready for review. The changes included are as follows :
|
fluiddot
reviewed
Mar 30, 2021
fluiddot
approved these changes
Mar 31, 2021
d865c2b
into
rnmobile/simplify_image_insertion_flow-redux-changes
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.
Description
The purpose of this PR is to refactor changes added in #29546 . A separate PR targeting that PR was utilized to reduce the diff changes the reviewer has to wrangle, so that the review can be done in more seamless manner. The main changes that were made were as a result of this feedback.
editorpackage to theblock-editorpackage.editorpackage to theblock-editorpackage.nativetowebto resolve e2e test issues of the base PR.Testing
Image 🟢
Video 🟢
Gallery 🔴
🛠 Error Details 🔴
To reproduce the error here, add another Gallery block after the successful addition of the first one and you will see this error.
I am thinking that the fact the
clearLastBlockInsertedin thegallery/edit.jsis causing the issue but I am not sure how to refactor so that the hook calls are consistent.Regression Testing
Types of changes
Review
Checklist: