-
Notifications
You must be signed in to change notification settings - Fork 3.7k
LHN list re-render #36927
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
LHN list re-render #36927
Changes from all commits
ac3f125
a247662
77df1e4
fc2b642
51e05c3
ad7bc16
8ebf567
0438c8a
462e200
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 |
|---|---|---|
| @@ -1,14 +1,11 @@ | ||
| import React from 'react'; | ||
| import useWindowDimensions from '@hooks/useWindowDimensions'; | ||
| import FreezeWrapper from '@libs/Navigation/FreezeWrapper'; | ||
| import BaseSidebarScreen from './BaseSidebarScreen'; | ||
| import sidebarPropTypes from './sidebarPropTypes'; | ||
|
|
||
| function SidebarScreen(props) { | ||
| const {isSmallScreenWidth} = useWindowDimensions(); | ||
|
|
||
| return ( | ||
| <FreezeWrapper keepVisible={!isSmallScreenWidth}> | ||
| <FreezeWrapper> | ||
|
Contributor
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. Why did you remove this? Is the sidebar still clickable if you e.g. navigate to different reports twice? I think it was handling a case where you have the sidebar visible all the time and navigate deeper in the main stack, but maybe the navigation structure changed in a way that it is not necessary right now?
Contributor
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. FYI I tested this fix locally and the list is still fully clickable on the web, and causes no issues on mobile. It seems plausible that the nav structure changed and this is no loner a case
Contributor
Author
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.
|
||
| <BaseSidebarScreen | ||
| // eslint-disable-next-line react/jsx-props-no-spreading | ||
| {...props} | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.