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 4 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
5 changes: 2 additions & 3 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 @@ -28,8 +27,8 @@ function FreezeWrapper({keepVisible = false, children}: FreezeWrapperProps) {
// 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 - (screenIndexRef.current ?? 0) > 1) {
InteractionManager.runAfterInteractions(() => setIsScreenBlurred(true));
hayata-suenaga marked this conversation as resolved.
Show resolved Hide resolved
if (navigation.getState().index - (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.

Does the issue with beginning of animation not occur anymore? And did you try to run it in the new arch? I think the case with InteractionManager was needed there only and I think we will want to enable it on web ASAP too.

Copy link
Contributor

@jbroma jbroma Feb 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I can't pinpoint the any issues with the animations. I haven't tested with new arch though.

Copy link
Contributor Author

@perunt perunt Feb 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Animation of goBack works ok for me. I haven't tried it with a new arch yet, but it shouldn't be an issue

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 also gave it a spin and wasn't able to experience anything weird related to animations.

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