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

update sign in page structure #36803

Merged
merged 6 commits into from
Feb 24, 2024
Merged

update sign in page structure #36803

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d37d2ad
update SignInPage structure
suneox Feb 15, 2024
4deab03
fix sign page typecheck
suneox Feb 19, 2024
76b746c
Merge remote-tracking branch 'upstream/main' into 36515-update-sign-i…
suneox Feb 19, 2024
a4fab45
fix prettier
suneox Feb 19, 2024
e469805
Merge remote-tracking branch 'upstream/main' into 36515-update-sign-i…
suneox Feb 21, 2024
80863fe
fix prettier
suneox Feb 21, 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
2 changes: 1 addition & 1 deletion src/pages/signin/SignInModal.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ function SignInModal() {
testID={SignInModal.displayName}
>
<HeaderWithBackButton onBackButtonPress={() => Navigation.goBack()} />
<SignInPage />
<SignInPage shouldEnableMaxHeight={false} />
</ScreenWrapper>
);
}
Expand Down
13 changes: 8 additions & 5 deletions src/pages/signin/SignInPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import Str from 'expensify-common/lib/str';
import React, {useEffect, useRef, useState} from 'react';
import {View} from 'react-native';
import {withOnyx} from 'react-native-onyx';
import type {OnyxEntry} from 'react-native-onyx';
import ColorSchemeWrapper from '@components/ColorSchemeWrapper';
import CustomStatusBarAndBackground from '@components/CustomStatusBarAndBackground';
import ScreenWrapper from '@components/ScreenWrapper';
import ThemeProvider from '@components/ThemeProvider';
import ThemeStylesProvider from '@components/ThemeStylesProvider';
import useLocalize from '@hooks/useLocalize';
Expand Down Expand Up @@ -48,7 +48,9 @@ type SignInPageInnerOnyxProps = {
preferredLocale: OnyxEntry<Locale>;
};

type SignInPageInnerProps = SignInPageInnerOnyxProps;
type SignInPageInnerProps = SignInPageInnerOnyxProps & {
shouldEnableMaxHeight?: boolean;
};

type RenderOption = {
shouldShowLoginForm: boolean;
Expand Down Expand Up @@ -124,7 +126,7 @@ function getRenderOptions({
};
}

function SignInPageInner({credentials, account, activeClients = [], preferredLocale}: SignInPageInnerProps) {
function SignInPageInner({credentials, account, activeClients = [], preferredLocale, shouldEnableMaxHeight = true}: SignInPageInnerProps) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking about this, I can't see a good reason to introduce shouldEnableMaxHeight as a prop as SignInPage isn't being used from anywhere else, so we could just set it to false here. Or am I missing something? 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

@suneox Friendly bump on this question 🙇

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Due to SignInPage use default at Router, and re-use at SignInModal so we should set default for a use case Router render

Copy link
Contributor

Choose a reason for hiding this comment

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

Did you test for that case? I think it would also want the same fix applied, no?

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 have test and also provide screenshot in Screenshots/Videos

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant have you tested for the case where the SignInModal is used in Router? I don't think I agree that it should have a different value for shouldEnableMaxHeight.

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 meant have you tested for the case where the SignInModal is used in Router

Yes i have tested both cases using SignInModal at

App/src/libs/Navigation/AppNavigator/PublicScreens.tsx

Lines 21 to 25 in 52b6d70

<RootStack.Screen
name={NAVIGATORS.BOTTOM_TAB_NAVIGATOR}
options={defaultScreenOptions}
component={SignInPage}
/>

and

App/src/pages/signin/SignInModal.tsx

Lines 22 to 30 in 52b6d70

<ScreenWrapper
style={[StyleUtils.getBackgroundColorStyle(theme.PAGE_THEMES[SCREENS.RIGHT_MODAL.SIGN_IN].backgroundColor)]}
includeSafeAreaPaddingBottom={false}
shouldEnableMaxHeight
testID={SignInModal.displayName}
>
<HeaderWithBackButton onBackButtonPress={() => Navigation.goBack()} />
<SignInPage />
</ScreenWrapper>

Screenshot 2024-02-22 at 23 28 31

I don't think I agree that it should have a different value for shouldEnableMaxHeight.

@jjcoffee
We have a different value for SignInPage due to at SignInModal has a HeaderWithBackButton and already set shouldEnableMaxHeight

so when SignInModal reuses a SignInPage as a component we should disable shouldEnableMaxHeight on SignInPage to avoid a double scroll bar (due to shouldEnableMaxHeight will get the screen height + HeaderWithBackButton).

const styles = useThemeStyles();
const StyleUtils = useStyleUtils();
const {translate, formatPhoneNumber} = useLocalize();
Expand Down Expand Up @@ -245,7 +247,8 @@ function SignInPageInner({credentials, account, activeClients = [], preferredLoc
return (
// Bottom SafeAreaView is removed so that login screen svg displays correctly on mobile.
// The SVG should flow under the Home Indicator on iOS.
<View
<ScreenWrapper
shouldEnableMaxHeight={shouldEnableMaxHeight}
style={[styles.signInPage, StyleUtils.getSafeAreaPadding({...safeAreaInsets, bottom: 0, top: isInModal ? 0 : safeAreaInsets.top}, 1)]}
testID={SignInPageInner.displayName}
>
Expand Down Expand Up @@ -281,7 +284,7 @@ function SignInPageInner({credentials, account, activeClients = [], preferredLoc
</>
)}
</SignInPageLayout>
</View>
</ScreenWrapper>
);
}

Expand Down
Loading