-
Notifications
You must be signed in to change notification settings - Fork 2.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix default drawer status issue #9076
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 @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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I asked this #8126 (comment) with Expected output #8126 (comment). 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
? 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 https://developer.mozilla.org/en-US/docs/Web/API/PerformanceNavigationTiming/type
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 I'm going to bring this to the Slack channel just to get some clarification. |
||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ import {View} from 'react-native'; | |
import withWindowDimensions, {windowDimensionsPropTypes} from '../../../components/withWindowDimensions'; | ||
import styles from '../../../styles/styles'; | ||
import * as StyleUtils from '../../../styles/StyleUtils'; | ||
import * as App from '../../actions/App'; | ||
|
||
import Navigation from '../Navigation'; | ||
|
||
|
@@ -66,6 +67,19 @@ class BaseDrawerNavigator extends Component { | |
), | ||
swipeEdgeWidth: 500, | ||
}} | ||
screenListeners={{ | ||
state: (e) => { | ||
const state = e.data.state; | ||
|
||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Yeah, you're right. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 https://reactnavigation.org/docs/navigation-state
So it seems like there should be at least something in the history at all times...
Does this seem correct? How can it happen? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry for not making it clear, what I mean by drawer history is history that has the type 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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... |
||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also don't understand this comment... where does the default status coming from ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The status comes from the route state. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 The Are these drawer states documented somewhere? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not well documented on the docs https://reactnavigation.org/docs/navigation-events/#state. Here are the example from
The |
||
App.setDefaultDrawerStatus(hasDrawerHistory ? hasDrawerHistory.status : state.default); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We save the drawer status when it changes. For example when we are on the report screen, so the drawer has the status There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 https://reactnavigation.org/docs/drawer-navigator#checking-if-the-drawer-is-open There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe it can be placed inside one of our main components like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, the hook is a good alternative, but in our cases, we can't use it on main components like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok well wherever we can put it and it will work seems more straightforward than this Let's maybe pause this briefly while we figure out if a drawer status tracker is necessary. |
||
}, | ||
}} | ||
> | ||
{_.map(this.props.screens, screen => ( | ||
<Drawer.Screen | ||
|
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -9,13 +9,20 @@ import CustomActions from './CustomActions'; | |||||||||
import ONYXKEYS from '../../ONYXKEYS'; | ||||||||||
import linkingConfig from './linkingConfig'; | ||||||||||
import navigationRef from './navigationRef'; | ||||||||||
import CONST from '../../CONST'; | ||||||||||
|
||||||||||
let isLoggedIn = false; | ||||||||||
Onyx.connect({ | ||||||||||
key: ONYXKEYS.SESSION, | ||||||||||
callback: val => isLoggedIn = Boolean(val && val.authToken), | ||||||||||
}); | ||||||||||
|
||||||||||
let defaultDrawerStatus = CONST.DRAWER_STATUS.OPEN; | ||||||||||
Onyx.connect({ | ||||||||||
key: ONYXKEYS.DEFAULT_DRAWER_STATUS, | ||||||||||
callback: val => defaultDrawerStatus = val, | ||||||||||
}); | ||||||||||
|
||||||||||
// This flag indicates that we're trying to deeplink to a report when react-navigation is not fully loaded yet. | ||||||||||
// If true, this flag will cause the drawer to start in a closed state (which is not the default for small screens) | ||||||||||
// so it doesn't cover the report we're trying to link to. | ||||||||||
|
@@ -74,9 +81,18 @@ function closeDrawer() { | |||||||||
*/ | ||||||||||
function getDefaultDrawerState(isSmallScreenWidth) { | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do not know the real reason for how this breaks it internally so I used the term 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 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ok haha well, if you don't know the reason as the author who will? 😅
Ok and we are not sure why the layout breaking on the drawer in the first place? Or did we discover the root cause?
Is there any open issue to figure out what the root cause is? Maybe would be good to create one issue that:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
😄 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.
I will find the ticket and details. Don't remember it correctly but it was either There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||||||
if (didTapNotificationBeforeReady) { | ||||||||||
return 'closed'; | ||||||||||
return CONST.DRAWER_STATUS.CLOSED; | ||||||||||
} | ||||||||||
return isSmallScreenWidth ? 'open' : 'closed'; | ||||||||||
|
||||||||||
if (isSmallScreenWidth) { | ||||||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yeah, I will update the comment to make it more make sense.
When the user opens from URL There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have suggestion in here. |
||||||||||
return path === '/' ? CONST.DRAWER_STATUS.OPEN : defaultDrawerStatus; | ||||||||||
} | ||||||||||
|
||||||||||
return CONST.DRAWER_STATUS.CLOSED; | ||||||||||
} | ||||||||||
|
||||||||||
/** | ||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice addition moving these to
CONST
, thanks!