[RNMobile] Simplify media insertion flow Part 1 - redux changes #29546
Conversation
jd-alexander
added
the
Mobile App - i.e. Android or iOS
|
Size Change: +144 B (0%) Total Size: 1.46 MB
ℹ️ View Unchanged
|
jd-alexander
changed the title
jd-alexander
changed the title
There was a problem hiding this comment.
This looks good to me. I don't have a strong performance, either way, regarding @guarani's suggestion so I'll leave that for others to comment on.
I'm totally happy with the way it is, I was just curious 👍 (so don't that comment hold this PR back @jd-alexander! 😄) I'm reviewing the other PRs now. |
|
BTW, the failing iOS E2E tests might have been fixed by #29589, so if you update with the latest from |
There might still be issues with these tests 😞 |
* 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
|
I just noticed the log contains a test failure due to changes made to the web gallery component. The error is in gallery.test.js I have started looking into it but I am not too sure what the resolution is because I am not familiar with the web side of e2e testing. I will pull in someone else if I don't see any clues tomorrow. |
|
Hi @ockham 👋🏾 would you be able to take a look at these failing e2e test and offer any guidance on how it could be resolved? When I was searching for clues in the repo, I saw where you had resolved a similar issue a while back. The current issue in these set of tests is
Back story: This PR introduces changes that allow several media related blocks to auto-open the I am wondering if there's some form of e2e fixture configurations that contains the jest mocks that can be utilized in these tests but so far I wasn't able to find them. Let me know if you have any ideas. Thanks much! |
I'm afraid that the issue I was facing the other day is of a fairly different kind. While they both share the "Expected mock function not to be called but it was called with..." part, that only tells us that a function was run that tests didn't expect to be run. In other words, the only thing both issues have in common is the way they are caught/reported. In "my" case, that unexpected function call was a block transformation ("Block successfully updated for |
|
Thanks for the feedback @ockham
This helps, as it confirms the comments made by @youknowriad that the I will work on making the changes needed to get things compliant. |
|
A refactor of the redux changes introduced in this PR is underway here #30123 Once the unit tests and code cleanups have been completed in that PR it will be merged back into this one, since this base PR targets |
* 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>
|
@fluiddot @guarani this is ready for the final review so that it can be merged. @guarani @fluiddot already reviewed the redux changes, and I also reviewed the improvements he made. Nonetheless, you can still give a second set of eyes and share any findings you may have. Apart from that, I think this can be merged once no other issues have been found. |
There was a problem hiding this comment.
I tested the changes on Android (device: Samsung Galaxy S20 FE) and iOS (device: iPhone 11) and works nice!
I didn't approve the PR because I found out a potential change, let me know what you think about it. Apart from that this PR is ready 🟢 , awesome work @jd-alexander 💯 !
| export function lastBlockInserted( state = {}, action ) { | ||
| switch ( action.type ) { | ||
| case 'INSERT_BLOCKS': | ||
| if ( ! action.updateSelection || ! action.blocks.length ) { |
There was a problem hiding this comment.
After a second thought, I think that we probably shouldn't include the ! action.updateSelection condition here. The last block inserted should be updated independently on if the block gets selected upon insertion, right?
Originally I thought that this was necessary because we only wanted to show the media picker when the block is selected, but I realised that we're already checking that in the components (example), what do you think?
There was a problem hiding this comment.
After a second thought, I think that we probably shouldn't include the ! action.updateSelection condition here. The last block inserted should be updated independently on if the block gets selected upon insertion, right?
when you say "updated independently on if the block gets selected upon insertion" what exactly do you mean? I want to ensure that I understand you clearly :)
Originally I thought that this was necessary because we only wanted to show the media picker when the block is selected, but I realised that we're already checking that in the components (example), what do you think?
Makes sense! I checked the linked example and it went to the withSelect in the Gallery component, so I am assuming that you are referring to the isSelected usage there. If that's the case, yes, the isSelected behavior was added before the redux approach was introduced, so if it's better to keep the selection logic within the reducer, I am not against that, especially if it will be consistent with other behavior in the reducer.
Another question, I wanted to add to the discussion. In which cases would the block be inserted and it wouldn't be a selection. I removed isSelected and updateSelection from the code in my local environs and the functionality still worked. I will look more into this tomorrow, but I just wanted to share my thoughts here, so you can add any ideas you may have.
There was a problem hiding this comment.
when you say "updated independently on if the block gets selected upon insertion" what exactly do you mean? I want to ensure that I understand you clearly :)
Oh sorry if I wasn't too clear, what I meant is that if this slice of the state contains the last block inserted, it makes sense that it gets updated for any block that is inserted, including the ones with the param updateSelection as false. From a consistence POV, I would expect that if I insert a block even if it doesn't get automatically selected (via the updateSelection action param), it would be set as the last block inserted.
Makes sense! I checked the linked example and it went to the withSelect in the Gallery component, so I am assuming that you are referring to the isSelected usage there. If that's the case, yes, the isSelected behavior was added before the redux approach was introduced, so if it's better to keep the selection logic within the reducer, I am not against that, especially if it will be consistent with other behavior in the reducer.
Yeah exactly, I was referring to the isSelected usage when calculating the value to pass to autoOpenMediaUpload prop.
In my opinion I think it would make more sense to keep the isSelected usage as we have currently in the components and update the lastBlockInserted reducer.
Another question, I wanted to add to the discussion. In which cases would the block be inserted and it wouldn't be a selection. I removed isSelected and updateSelection from the code in my local environs and the functionality still worked. I will look more into this tomorrow, but I just wanted to share my thoughts here, so you can add any ideas you may have.
I think it's a uncommon case but looks like there's at least one place where it's being used in the Jetpack project:
https://github.com/Automattic/jetpack/blob/5b637a82a9836213f066809cab550ce3590921cb/projects/plugins/jetpack/extensions/blocks/anchor-fm/editor.js#L48
There was a problem hiding this comment.
Oh sorry if I wasn't too clear, what I meant is that if this slice of the state contains the last block inserted, it makes sense that it gets updated for any block that is inserted, including the ones with the param updateSelection as false. From a consistence POV, I would expect that if I insert a block even if it doesn't get automatically selected (via the updateSelection action param), it would be set as the last block inserted.
Ah I get what you are saying now. Thanks for the clarification 🙇🏾
In my opinion I think it would make more sense to keep the isSelected usage as we have currently in the components and update the lastBlockInserted reducer.
I agree with this as well after gaining a deeper understanding due to your clarifications above. I will make the relevant changes to support this.
I think it's a uncommon case but looks like there's at least one place where it's being used in the Jetpack project:
https://github.com/Automattic/jetpack/blob/5b637a82a9836213f066809cab550ce3590921cb/projects/plugins/jetpack/extensions/blocks/anchor-fm/editor.js#L48
Good find 😉 Thanks for checking Jetpack. If I had a sleuthing badge 🔍 I would definitely give you one 😄
There was a problem hiding this comment.
I agree with this as well after gaining a deeper understanding due to your clarifications above. I will make the relevant changes to support this.
Nice! Thanks for making the changes, I'll be watching them so we can have this PR ready as soon as possible.
Good find 😉 Thanks for checking Jetpack. If I had a sleuthing badge 🔍 I would definitely give you one 😄
Thank you very much! 🕵️😄
jd-alexander
deleted the
rnmobile/simplify_image_insertion_flow-redux-changes
branch
gutenberg-mobilePR wordpress-mobile/gutenberg-mobile#2700Description
These changes are related to the PR #25031 that is being broken down into separate PRs for easier testing and review.
Once this PR has been approved, we can merge the other two PRs into this one and then merge all the changes into
trunk.The
gutenberg-mobilePR wordpress-mobile/gutenberg-mobile#2700 contains a temporary ref to the last PR for testing purposes.The main goal of this PR is to introduce changes to the native redux store behavior that will facilitate the storing of the last block that was inserted i.e the last block the user clicked on the
Insertermenu. This solution was utilized so that aImage, Video or Gallerywould be able to query the store for theclientIdof the last block inserted if there is a match then there can be a resulting action.How has this been tested?
npm run native test.Componentlevel will showcase that the functionality works as expected.Types of changes
Changes were made to the native variants of the actions, reducer, and selectors.
Checklist: