Improve react-navigation CustomAction#6581
Conversation
| * @returns {Object} | ||
| */ | ||
| function getParamsFromState(state) { | ||
| return lodashGet(state, 'routes[0].state.routes[0].params', {}); |
There was a problem hiding this comment.
This could be wrong. As the active route is decided via the index property on the state. I think it should be recursive until we get the final Active state. there could be multiple nested states.
In our case, there are only two level.
Stack
=> Drawer || Stack
There was a problem hiding this comment.
Are you saying it could be wrong when we use the state returned from getStateFromPath()?
Or are you saying the existing logic had some problem and could be wrong?
What should I do to observe this?
There was a problem hiding this comment.
What I observed from this code is that you are trying to get the params of the active route.
If you are meant to do that then
So this statement routes[0].state.routes[0].params is not necessarily giving us params of active route. Under the state object, any route could be active from all items in the routes array. The state has an index property that tells which one is the active one.
state.routes[state.index ?? 0] I saw similar code in the react-navigation codebase.
Now the route you get from the above step can have a nested state which will follow on. So I am suggesting getting it via recursion.
There was a problem hiding this comment.
I think I understand... but there are only two cases where we try to use the state currently:
- active route which is the just using the root state (logic should be the same as before - was it wrong?)
- current route which we were previously getting the params and screen name from the custom
navigate(), but it seemed better to extract that out ofgetStateFromPath()(which I don't think returns any nested state for the report case)
So which case do we need to use recursion for? Is there something I can do in the app to break the current logic and see that what we have isn't working correctly?
There was a problem hiding this comment.
It seems that it just works for our cases. Probably we don't need to get that deeper into it. I can't think of any cases in our App where this logic won't work as this is extracted from the previous code.
There was a problem hiding this comment.
Ok gonna leave a note somewhere about this - but maybe not worry about a solution. The custom action will probably need to evolve as time goes on and navigators get more complex.
| * @returns {String} | ||
| */ | ||
| function getScreenNameFromState(state) { | ||
| return lodashGet(state, 'routes[0].state.routes[0].name', ''); |
|
BTW, loved the clean-up here and it started to sound meaningful now. |
Luke9389
left a comment
Luke9389
left a comment
There was a problem hiding this comment.
Big fan of these changes. Thanks @marcaaron
alex-mechler
left a comment
alex-mechler
left a comment
There was a problem hiding this comment.
A lot cleaner. Thanks for leaving good comments throughout
@parasharrajat did you want to review again?
| const history = _.map(state.history ? [...state.history] : [screenRoute], () => screenRoute); | ||
|
|
||
| // Force drawer to close and show the report screen | ||
| // Force drawer to close and show |
There was a problem hiding this comment.
NAB: I think this is removed mistakenly or and show can be removed as well.
| // Force drawer to close and show | |
| // Force drawer to close |
There was a problem hiding this comment.
Ah yea, not a mistake because I didn't want to have the "report page" referenced here.
There was a problem hiding this comment.
Oh I see what you mean actually. It does kind of seem like an unfinished thought... "and show... what?"
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by @marcaaron in version: 1.1.19-5 🚀
|
|
🚀 Deployed to production by @Julesssss in version: 1.1.21-1 🚀
|
5 participants
Details
cc @parasharrajat - trying to document better the logic in our CustomAction and make it more generic
Fixed Issues
None
Tests / QA Steps
Same as #6207
Tested On