Skip to content
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.

Sign up for GitHub

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

Jump to bottom

LHN list re-render #36927

Merged
merged 9 commits into from
Mar 12, 2024
Merged

LHN list re-render #36927

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ac3f125
Merge branch 'main' of https://github.com/Expensify/App
perunt Feb 19, 2024
a247662
Merge branch 'main' of https://github.com/Expensify/App
perunt Feb 19, 2024
77df1e4
remove delay in freeze
perunt Feb 20, 2024
fc2b642
remove keepVisible
perunt Feb 20, 2024
51e05c3
Merge branch 'main' of https://github.com/margelo/expensify-app-fork …
perunt Mar 4, 2024
ad7bc16
Merge branch 'main' of https://github.com/Expensify/App into perunt/p…
perunt Mar 4, 2024
8ebf567
clean comment
perunt Mar 11, 2024
0438c8a
Merge branch 'main' of https://github.com/Expensify/App into perunt/p…
perunt Mar 11, 2024
462e200
bring back freezing on 1 screen away
perunt Mar 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 2 additions & 6 deletions src/libs/Navigation/FreezeWrapper.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import {useIsFocused, useNavigation, useRoute} from '@react-navigation/native';
import React, {useEffect, useRef, useState} from 'react';
import {Freeze} from 'react-freeze';
import {InteractionManager} from 'react-native';
import type ChildrenProps from '@src/types/utils/ChildrenProps';

type FreezeWrapperProps = ChildrenProps & {
Expand All @@ -25,11 +24,8 @@ function FreezeWrapper({keepVisible = false, children}: FreezeWrapperProps) {

useEffect(() => {
const unsubscribe = navigation.addListener('state', () => {
// if the screen is more than 1 screen away from the current screen, freeze it,
// we don't want to freeze the screen if it's the previous screen because the freeze placeholder
// would be visible at the beginning of the back animation then
if ((navigation.getState()?.index ?? 0) - (screenIndexRef.current ?? 0) > 1) {
InteractionManager.runAfterInteractions(() => setIsScreenBlurred(true));
hayata-suenaga marked this conversation as resolved.
Show resolved Hide resolved
if ((navigation.getState()?.index ?? 0) - (screenIndexRef.current ?? 0) >= 1) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that you changed to >= 1 from > 1. Isn't this opposed to this comment?

    // we don't want to freeze the screen if it's the previous screen because the freeze placeholder
    // would be visible at the beginning of the back animation then

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that we no longer use the placeholder component. I guess this comment was not changed after the placeholder was removed. I will do it now

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@perunt this actually caused regression.
As this affects all platforms, please test and add remaining platforms, not only web.

this branch:

Screen.Recording.2024-03-11.at.10.54.20.PM.mov

main:

Screen.Recording.2024-03-11.at.10.54.42.PM.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@situchan, thanks for pointing it out.
I retested it, and it looks like a freshly added nullish coalescing operator can let me remove this check, so I assume we can't receive navigation immediately in some cases. I'm adding the rest of the recordings.

setIsScreenBlurred(true);
} else {
setIsScreenBlurred(false);
}
Expand Down
5 changes: 1 addition & 4 deletions src/pages/home/sidebar/SidebarScreen/index.js
View file
Open in desktop
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>
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keepVisible completely disables freezing for a 'big' screen. I believe the original idea of this chunk was to freeze it directly in such cases. I wonder why we used this approach before. It might be a leftover from the previous navigation structure

<BaseSidebarScreen
// eslint-disable-next-line react/jsx-props-no-spreading
{...props}
Expand Down
Loading