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.

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: LoginPage for wide screens #6137

Merged
merged 8 commits into from
Dec 7, 2021
Merged

Conversation

parasharrajat
Copy link
Member

@parasharrajat parasharrajat commented Oct 31, 2021

Details

Use KeyboardAvoidingView to push the input into the viewport and Scrollview to Allow the user to scroll the screen.

Fixed Issues

$ #5159

Tests

  1. Open the Login page on a tablet or Ipad.
  2. Click on login input and see it remains in the View and is not hidden by Keyboard.

  1. Open the Password Page on Tablet or Ipad.
  2. Clicked all the inputs to see if they are not hidden behind the keyboard.

  1. Open the Login page on landscape Mode.
  2. Clicked all the inputs to see if they are not hidden behind the keyboard.

QA Steps

  1. Open Login, Password pages on Landscape Mode or tablet or Ipad.
  2. Observe the input should not be hidden behind the keyboard.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web | Desktop

image

Mobile Web

Screenshot 2021-12-07 01:42:23

iOS

Android

Screenshot 2021-12-07 01:37:36

output_file.mp4

@parasharrajat parasharrajat marked this pull request as ready for review December 6, 2021 21:23
@parasharrajat parasharrajat requested a review from a team as a code owner December 6, 2021 21:23
@MelvinBot MelvinBot removed the request for review from a team December 6, 2021 21:23
@botify botify requested a review from Luke9389 December 6, 2021 21:23
Comment on lines +48 to +54
<Form style={[
styles.flex1,
styles.alignSelfStretch,
props.isSmallScreenWidth && styles.signInPageNarrowContentContainer,
props.isSmallScreenWidth ? styles.ph5 : styles.ph4,
]}
>
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation looks a little weird here. Maybe ]}>?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, it's totally fine. There are two ways here.
This one
e.g.

<View style={[
(this.state.isFocused || this.state.isDraggingOver)
? styles.chatItemComposeBoxFocusedColor
: styles.chatItemComposeBoxColor,
styles.chatItemComposeBox,
styles.flexRow,
]}
>

and

        <Form
            style={[
                styles.flex1,
                styles.alignSelfStretch,
                props.isSmallScreenWidth && styles.signInPageNarrowContentContainer,
                props.isSmallScreenWidth ? styles.ph5 : styles.ph4,
            ]}
        >

Copy link
Contributor

Choose a reason for hiding this comment

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

Huh, Ok well that's fine with me. This was a NAB.

return content;
}

return <SignInPageWideContainer>{content}</SignInPageWideContainer>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there really an advantage to having the SignInPageWideContainer be its own component? I think it'd be better for us to get rid of the SignInPageWideContainer altogether and have the View and SVG components it contains live here in this file instead. In other words, why have these as two files when it's not really complicated to have them be one?

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks clean this way but let me know I can merge.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just merged. Thanks for the suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, I appreciate your willingness to do that. 👍

@Luke9389 Luke9389 merged commit 838704a into Expensify:main Dec 7, 2021
@OSBotify
Copy link
Contributor

OSBotify commented Dec 7, 2021

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

OSBotify commented Dec 8, 2021

🚀 Deployed to staging by @Luke9389 in version: 1.1.18-4 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @Julesssss in version: 1.1.21-1 🚀

platform result
🤖 android 🤖 failure ❌
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants