fix default drawer status issue #9076
Conversation
melvin-bot
bot
requested review from
parasharrajat and
thienlnam
and removed request for
a team
There was a problem hiding this comment.
I'm a bit concerned with these changes. To be honest, not sure if I fully understand what the root cause of this issue is - but it just feels like something we shouldn't need so much custom logic to enable. Did we get anywhere when we raised a conversation with the react-navigation maintainers?
I have brought this up in the past, but seems like my advice keeps getting ignored as more workarounds pop up to support weird navigational edge cases.
| // Get the history that has drawer type to get the status. | ||
| const hasDrawerHistory = _.find(state.history || [], h => h.type === 'drawer'); | ||
|
|
||
| // hasDrawerHistory will has undefined value if the route drawer is equal to initial route drawer. |
There was a problem hiding this comment.
I don't really understand this. What does "if the route drawer is equal to initial route drawer" mean exactly?
There was a problem hiding this comment.
Also, hasDrawerHistory is not a boolean so this variable name should communicate that this is a "first history item that is type drawer"? (If I am understanding this correctly - not really getting any of this code)
There was a problem hiding this comment.
When we first open the page and shows up the LHN, then click on any report item, when on the report screen the drawer has the history. But when we go back again to LHN, the drawer history will return undefined.
Also, hasDrawerHistory is not a boolean so this variable name should communicate that this is a "first history item that is type drawer"? (If I am understanding this correctly - not getting any of this code)
Yeah, you're right.
There was a problem hiding this comment.
Thanks for the additional thoughts! I'm trying to better understand what "history" is...
Looking at the shape of a navigation state object in the react-navigation docs and found this:
https://reactnavigation.org/docs/navigation-state
history- A list of visited items. This is an optional property and not present in all navigators. For example, it's only present in tab and drawer navigators in the core. The shape of the items in the history array can vary depending on the navigator. There should be at least one item present in this array.
So it seems like there should be at least something in the history at all times...
when we go back again to LHN, the drawer history will return undefined.
Does this seem correct? How can it happen?
There was a problem hiding this comment.
Sorry for not making it clear, what I mean by drawer history is history that has the type drawer.
For example on this screenshot, when I first open the app it's showing a report screen. Then I go to LHN, in this step the history will have a history that has the type drawer which has open value. After that, I open the report screen again and there's no history for type drawer.
There was a problem hiding this comment.
Ok so the history always has a value - it's just not always a type "drawer" history item. That would be good to clarify in the comment.
But maybe let's determine if this is the correct solution first...
|
|
||
| // hasDrawerHistory will has undefined value if the route drawer is equal to initial route drawer. | ||
| // Using the default status if hasDrawerHistory is undefined and get the status from the current route | ||
| // if the value provided. |
There was a problem hiding this comment.
Also don't understand this comment... where does the default status coming from ?
There was a problem hiding this comment.
The status comes from the route state.
There was a problem hiding this comment.
Sorry, I'm not sure if I'm following that...
Going to try to explain in my own words and you let me know if I'm on the right track...
The item that we are getting from the history is a route. That route has a status that refers to the state of the drawer.
The e.data.state has a default property to indicate the drawer status as well.
Are these drawer states documented somewhere?
There was a problem hiding this comment.
It's not well documented on the docs https://reactnavigation.org/docs/navigation-events/#state.
Here are the example from data.state
{
"stale": false,
"type": "drawer",
"key": "drawer-o_B0nHlaCG0jGCWESf5PA",
"index": 0,
"routeNames": [
"Report"
],
"history": [
{
"type": "route",
"name": "Report"
},
{
"type": "route",
"name": "Report"
},
{
"type": "route",
"name": "Report"
}
],
"routes": [
{
"name": "Report",
"params": {
"reportID": "90420013"
},
"key": "Report-oiQ55dmXeZgHMcLsx0rmh"
}
],
"default": "closed"
}
{
"stale": false,
"type": "drawer",
"key": "drawer-o_B0nHlaCG0jGCWESf5PA",
"index": 0,
"routeNames": [
"Report"
],
"history": [
{
"type": "route",
"name": "Report"
},
{
"type": "route",
"name": "Report"
},
{
"type": "drawer",
"status": "open"
}
],
"routes": [
{
"name": "Report",
"params": {
"reportID": "92055915"
},
"key": "Report-YVI0AsQPNmKDO-oKjc-Sl"
}
],
"default": "closed"
}
The default value is what we set from defaultStatus props of Drawer.Navigator.
| // hasDrawerHistory will has undefined value if the route drawer is equal to initial route drawer. | ||
| // Using the default status if hasDrawerHistory is undefined and get the status from the current route | ||
| // if the value provided. | ||
| App.setDefaultDrawerStatus(hasDrawerHistory ? hasDrawerHistory.status : state.default); |
There was a problem hiding this comment.
When do we need this to run exactly? It seems like it will happen on every state change...
There was a problem hiding this comment.
The bug seems like it just affects the initial state of the drawer when the app first inits so do we need to set this any other time?
There was a problem hiding this comment.
We save the drawer status when it changes.
For example when we are on the report screen, so the drawer has the status closed, and then we save the status to Onyx. When we refresh the page, it will still be showing the report screen, not the LHN.
There was a problem hiding this comment.
Ok so to summarize, this code is saving the "last state of the drawer" so that when we open the app again we can make sure to use that drawer state?
First question I have is whether it would be better to use the useDrawerStatus() hook to track this?
https://reactnavigation.org/docs/drawer-navigator#checking-if-the-drawer-is-open
There was a problem hiding this comment.
Maybe it can be placed inside one of our main components like Expensify and update the status there.
There was a problem hiding this comment.
Yeah, the hook is a good alternative, but in our cases, we can't use it on main components like Expensify because the component is not the child of the Drawer.Navigator. If we still want to use the hook, we need to add it to the screens component that uses the drawer navigation.
There was a problem hiding this comment.
Ok well wherever we can put it and it will work seems more straightforward than this screenListeners thing that is looking at a bunch of undocumented parameters that could change in the future?
Let's maybe pause this briefly while we figure out if a drawer status tracker is necessary.
| const path = getPathFromState(navigationRef.current.getState(), linkingConfig.config); | ||
|
|
||
| // If the initial route path is HOME SCREEN, | ||
| // return open for default status drawer instead of using value from Onyx |
There was a problem hiding this comment.
suggestion: This comment should explain why the "home" route will always return a status of "open"
But that kind of begs the question of why defaultDrawerStatus is not already the correct value?
There was a problem hiding this comment.
suggestion: This comment should explain why the "home" route will always return a status of "open"
Yeah, I will update the comment to make it more make sense.
But that kind of begs the question of why defaultDrawerStatus is not already the correct value?
When the user opens from URL https://staging.new.expensify.com I make it will open the LHN even if the latest drawer status is closed.
There was a problem hiding this comment.
When the user opens from URL https://staging.new.expensify.com I make it will open the LHN even if the latest drawer status is closed.
That makes sense. But wouldn't the opposite be true as well? If we are directly navigating to a "report route" then the drawer should always be closed.
I'm not sure I understand the case where we need to track and save the "last drawer status"?
Seems like if we did the following there would be no issue:
When the url is /r/<reportID> drawer should init closed
When the url is / drawer should init open
?
There was a problem hiding this comment.
I have suggestion in here.
| DRAWER_STATUS: { | ||
| OPEN: 'open', | ||
| CLOSED: 'closed', | ||
| }, |
There was a problem hiding this comment.
Nice addition moving these to CONST, thanks!
| @@ -193,4 +193,7 @@ export default { | |||
|
|
|||
| // Validating Email? | |||
| USER_SIGN_UP: 'userSignUp', | |||
|
|
|||
| // Store default drawer status | |||
| DEFAULT_DRAWER_STATUS: 'defaultDrawerStatus', | |||
There was a problem hiding this comment.
thought: unsure whether we need to store this in Onyx or not... seems like something that could just be local to the navigation lib.
There was a problem hiding this comment.
The reason why we use Onyx is because we need a place that can keep the value even after refresh the page.
There was a problem hiding this comment.
But why do we need to keep the value after a refresh of the page?
What is the expected behavior if someone directly navigates to a /r/<reportID> route? Would we show the report or the sidebar?
@parasharrajat do you know the expected behavior and can you point me to where we decided how it should work?
There was a problem hiding this comment.
I asked this #8126 (comment) with Expected output #8126 (comment).
It should show the report screen.
But the main issue was that If the user is viewing LHN, refreshing it should land back on LHN and vice-versa.
There was a problem hiding this comment.
Yeah I guess my question could be re-phrased as... do we need this feature to track the drawer state at all?
What is the issue with this logic?
- When
/-> Show drawer open - When
/r/123-> Show report page (drawer closed)
?
Switching to the sidebar and refreshing the page e.g. will remember that the drawer is open when we are on the report page. Does this mean in other contexts when I want to navigate directly to a report page it will instead show me the sidebar if it was last set as 'open'? That sounds undesirable.
There was a problem hiding this comment.
Yes, you're right.
I have another approach for that case, but it's not 100% working as expected.
On my Chrome Version 101.0.4951.64 (Official Build) (arm64) there is a case if we paste the same URL as the current URL it will read as reload type, other than that can be returned as navigate type. On Safari, it works as expected.
https://developer.mozilla.org/en-US/docs/Web/API/PerformanceNavigationTiming/type
if ((window.performance.navigation && window.performance.navigation.type === 1) || _.map(window.performance.getEntriesByType('navigation'), nav => nav.type).includes('reload')) {
return defaultDrawerStatus;
}
const path = getPathFromState(navigationRef.current.getState(), linkingConfig.config);
return path === '/' ? CONST.DRAWER_STATUS.OPEN : CONST.DRAWER_STATUS.CLOSED;
There was a problem hiding this comment.
Ah sorry, if I'm understanding the proposal... you are saying that we should differentiate between a "reload" and a "navigate" - but I'm not sure why.
Seems like determining the default state of the drawer is only necessary when a new app window opens or is reloaded.
Overall, it feels like the drawer status is something that the linkingConfig in react-navigation should handle and be based not on history - but on the url.
I'm going to bring this to the Slack channel just to get some clarification.
| @@ -74,9 +81,18 @@ function closeDrawer() { | |||
| */ | |||
| function getDefaultDrawerState(isSmallScreenWidth) { | |||
There was a problem hiding this comment.
I'm not really getting this method or why we are using it...
Found this...
App/src/libs/Navigation/AppNavigator/BaseDrawerNavigator.js
Lines 45 to 48 in f2218b6
Which led me here:
The comment there makes no sense at all to me and vaguely references the "navigation internal state". @parasharrajat @thienlnam what exactly is this code doing?
There was a problem hiding this comment.
I do not know the real reason for how this breaks it internally so I used the term navigation internal state but I found the following:
This component uses a key prop to remount the navigator when the browser resizes. This hack was done to prevent the layout from breaking on the drawer. Now due to that, it is somehow breaking the internal navigation state on change of defaultStatus. So instead of passing the prop directly to the navigator, I calculated it once per mount and passed. It seems to work at that time. I moved forward with this change as we can't remove the key hack. It breaks the UI for Drawer.
So in short, we can remove the key prop hack, we can get rid of this.
So as we are trying to move away from custom Logic. I think an audit should be done of our navigation implementation and bad patterns or hacks should be removed. I can take a look at that but not sure if I will be able to suggest a better design.
There was a problem hiding this comment.
I do not know the real reason
Ok haha well, if you don't know the reason as the author who will? 😅
This component uses a key prop to remount the navigator when the browser resizes. This hack was done to prevent the layout from breaking on the drawer.
Ok and we are not sure why the layout breaking on the drawer in the first place? Or did we discover the root cause?
in short, we can remove the key prop hack, we can get rid of this.
Is there any open issue to figure out what the root cause is? Maybe would be good to create one issue that:
- Documents the workaround we took and why with as much information as possible
- Asks to investigate the root cause
- Try to fix it in
react-navigation
There was a problem hiding this comment.
Ok haha well, if you don't know the reason as the author who will?
😄 I debugged the internal state during analyzing that issue and found that it was causing the wrong status ('open' | 'closed') on the history. The drawer state (closed or open) depends on status history entries. And the right value ('open' | 'closed') in the history entry depends on how you configure the defaultStatus prop. During my refactoring, I found that changing is dynamically set the wrong value (But don't know how). It was a workaround.
Is there any open issue to figure out what the root cause is? Maybe would be good to create one issue that:
I will find the ticket and details. Don't remember it correctly but it was either withWindowDimensions issue or Reanimated 1.
There was a problem hiding this comment.
There was a problem hiding this comment.
Maybe we can create a new issue to get to the root cause of #5591 ? Temporary solution you have there seems fine for now, but could have been documented better.
|
Closed! #8126 (comment) |
Details
Get the drawer history from
screenListenersonDrawer.Navigatorcomponent, then save the drawer status from drawer history if provided to Onyx. We also returnopenif the drawer route path isHOME SCREEN.Fixed Issues
$ #8126
Tests
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)PR Reviewer Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesSTYLE.md) were followed/** comment above it */displayNamepropertythisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)QA Steps
Screenshots
Web
Screen.Recording.2022-05-18.at.23.42.04.mov
Mobile Web
screen-recording-2022-05-18-at-231817_tZxTdaAe.mp4
screen-recording-2022-05-18-at-231900_7nrVc4ne.mp4
Desktop
Screen.Recording.2022-05-18.at.23.53.05.mov
iOS
Screen.Recording.2022-05-18.at.23.58.17.mov
Android
screen-recording-2022-05-19-at-000543_gXBNFp1M.mp4