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 the keyboard overlapping buttons on the sign in form #12848

Merged
merged 17 commits into from
Dec 5, 2022
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
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/components/FormAlertWithSubmitButton.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ const defaultProps = {

const FormAlertWithSubmitButton = props => (
<FormAlertWrapper
containerStyles={[styles.mh5, styles.mb5, styles.flex1, styles.justifyContentEnd, ...props.containerStyles]}
containerStyles={[styles.mh5, styles.mb5, styles.justifyContentEnd, ...props.containerStyles]}
isAlertVisible={props.isAlertVisible}
isMessageHtml={props.isMessageHtml}
message={props.message}
Expand Down
7 changes: 7 additions & 0 deletions src/components/withWindowDimensions.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ const windowDimensionsPropTypes = {

// Is the window width narrow, like on a tablet device?
isMediumScreenWidth: PropTypes.bool.isRequired,

// Is the window width wide, like on a browser or desktop?
isLargeScreenWidth: PropTypes.bool.isRequired,
};

const windowDimensionsProviderPropTypes = {
Expand All @@ -35,6 +38,7 @@ class WindowDimensionsProvider extends React.Component {
const isSmallScreenWidth = initialDimensions.width <= variables.mobileResponsiveWidthBreakpoint;
const isMediumScreenWidth = initialDimensions.width > variables.mobileResponsiveWidthBreakpoint
&& initialDimensions.width <= variables.tabletResponsiveWidthBreakpoint;
const isLargeScreenWidth = !isSmallScreenWidth && !isMediumScreenWidth;

this.dimensionsEventListener = null;

Expand All @@ -43,6 +47,7 @@ class WindowDimensionsProvider extends React.Component {
windowWidth: initialDimensions.width,
isSmallScreenWidth,
isMediumScreenWidth,
isLargeScreenWidth,
};
}

Expand All @@ -67,11 +72,13 @@ class WindowDimensionsProvider extends React.Component {
const {window} = newDimensions;
const isSmallScreenWidth = window.width <= variables.mobileResponsiveWidthBreakpoint;
const isMediumScreenWidth = !isSmallScreenWidth && window.width <= variables.tabletResponsiveWidthBreakpoint;
const isLargeScreenWidth = !isSmallScreenWidth && !isMediumScreenWidth;
this.setState({
windowHeight: window.height,
windowWidth: window.width,
isSmallScreenWidth,
isMediumScreenWidth,
isLargeScreenWidth,
});
}

Expand Down
2 changes: 1 addition & 1 deletion src/pages/signin/ResendValidationForm.js
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ const ResendValidationForm = (props) => {
{!_.isEmpty(props.account.message) && (

// DotIndicatorMessage mostly expects onyxData errors so we need to mock an object so that the messages looks similar to prop.account.errors
<DotIndicatorMessage style={[styles.mb5]} type="success" messages={{0: props.account.message}} />
<DotIndicatorMessage style={[styles.mb5, styles.flex0]} type="success" messages={{0: props.account.message}} />
)}
{!_.isEmpty(props.account.errors) && (
<DotIndicatorMessage style={[styles.mb5]} type="error" messages={props.account.errors} />
Expand Down
76 changes: 28 additions & 48 deletions src/pages/signin/SignInPageLayout/SignInPageContent.js
Original file line number Diff line number Diff line change
@@ -1,20 +1,17 @@
import React from 'react';
import {ScrollView, View} from 'react-native';
import {View, TouchableWithoutFeedback, Keyboard} from 'react-native';
import PropTypes from 'prop-types';
import {withSafeAreaInsets} from 'react-native-safe-area-context';
import styles from '../../../styles/styles';
import variables from '../../../styles/variables';
import KeyboardAvoidingView from '../../../components/KeyboardAvoidingView';
import ExpensifyCashLogo from '../../../components/ExpensifyCashLogo';
import Text from '../../../components/Text';
import TermsAndLicenses from '../TermsAndLicenses';
import withLocalize, {withLocalizePropTypes} from '../../../components/withLocalize';
import SignInPageForm from '../../../components/SignInPageForm';
import compose from '../../../libs/compose';
import scrollViewContentContainerStyles from './signInPageStyles';
import withKeyboardState from '../../../components/withKeyboardState';
import withWindowDimensions, {windowDimensionsPropTypes} from '../../../components/withWindowDimensions';
import * as StyleUtils from '../../../styles/StyleUtils';

const propTypes = {
/** The children to show inside the layout */
Expand All @@ -31,46 +28,29 @@ const propTypes = {
...windowDimensionsPropTypes,
};

const SignInPageContent = props => (
<ScrollView
keyboardShouldPersistTaps="handled"
showsVerticalScrollIndicator={false}
style={[
styles.h100,
!props.isSmallScreenWidth && styles.alignSelfCenter,
!props.isSmallScreenWidth && styles.signInPageWideLeftContainer,
]}
contentContainerStyle={[
scrollViewContentContainerStyles,
styles.alignItemsCenter,
!props.isSmallScreenWidth && styles.ph6,
]}
>
<View style={[styles.flex1, styles.flexRow]}>
<View style={[
styles.flex1,
styles.signInPageNarrowContentContainer,
]}
>
<SignInPageForm style={[
const SignInPageContent = (props) => {
const dismissKeyboardWhenTappedOutsideOfInput = () => {
// This prop comes from withKeyboardState
if (!props.isShown) {
return;
}
Keyboard.dismiss();
};

return (
<TouchableWithoutFeedback onPress={dismissKeyboardWhenTappedOutsideOfInput}>
<View
style={[
styles.flex1,
styles.alignSelfStretch,
props.isSmallScreenWidth ? styles.ph5 : styles.ph4,
styles.signInPageLeftContainer,
styles.signInPageLeftContainerMediumScreen,
]}
>
<KeyboardAvoidingView
behavior="position"
style={[
StyleUtils.getModalPaddingStyles({
shouldAddBottomSafeAreaPadding: true,
modalContainerStylePaddingBottom: 20,
safeAreaPaddingBottom: props.insets.bottom,
}),
props.isSmallScreenWidth ? styles.signInPageNarrowContentMargin : {},
!props.isMediumScreenWidth || (props.isMediumScreenWidth && props.windowHeight < variables.minHeightToShowGraphics) ? styles.signInPageWideLeftContentMargin : {},
styles.mb3,
]}
>
>
{/* This empty view creates margin on the top of the sign in form which will shrink and grow depending on if the keyboard is open or not */}
<View style={[styles.flexGrow1, styles.signInPageContentTopSpacer]} />

<View style={[styles.flexGrow2]}>
<SignInPageForm style={[styles.alignSelfStretch]}>
<View style={[
styles.componentHeightLarge,
...(props.isSmallScreenWidth ? [styles.mb2] : [styles.mt6, styles.mb5]),
Expand All @@ -87,15 +67,15 @@ const SignInPageContent = props => (
</Text>
)}
{props.children}
</KeyboardAvoidingView>
</SignInPageForm>
<View style={[styles.mb5, styles.alignSelfCenter, styles.ph5]}>
</SignInPageForm>
</View>
<View style={[styles.mv5, styles.alignSelfCenter, !props.isLargeScreenWidth && styles.signInPageWideLeftContainer]}>
<TermsAndLicenses />
</View>
</View>
</View>
</ScrollView>
);
</TouchableWithoutFeedback>
);
};

SignInPageContent.propTypes = propTypes;
SignInPageContent.displayName = 'SignInPageContent';
Expand Down
11 changes: 1 addition & 10 deletions src/pages/signin/SignInPageLayout/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import PropTypes from 'prop-types';
import SignInPageContent from './SignInPageContent';
import withWindowDimensions, {windowDimensionsPropTypes} from '../../../components/withWindowDimensions';
import styles from '../../../styles/styles';
import variables from '../../../styles/variables';
import SignInPageGraphics from './SignInPageGraphics';

const propTypes = {
Expand All @@ -25,29 +24,21 @@ const SignInPageLayout = (props) => {
let containerStyles = [styles.flex1, styles.signInPageInner];
let contentContainerStyles = [styles.flex1, styles.flexRow];

const isLongMediumScreenWidth = props.isMediumScreenWidth && props.windowHeight >= variables.minHeightToShowGraphics;

if (props.isSmallScreenWidth) {
containerStyles = [styles.flex1];
contentContainerStyles = [styles.flex1];
} else if (isLongMediumScreenWidth) {
containerStyles = [styles.dFlex, styles.signInPageInner, styles.flexColumnReverse, styles.justifyContentBetween];
contentContainerStyles = [styles.flex1];
}

return (
<View style={containerStyles}>
{isLongMediumScreenWidth && (
<SignInPageGraphics />
)}
<View style={contentContainerStyles}>
<SignInPageContent
welcomeText={props.welcomeText}
shouldShowWelcomeText={props.shouldShowWelcomeText}
>
{props.children}
</SignInPageContent>
{!props.isSmallScreenWidth && !isLongMediumScreenWidth && (
{!props.isSmallScreenWidth && (
<SignInPageGraphics />
)}
</View>
Expand Down
53 changes: 10 additions & 43 deletions src/styles/styles.js
Original file line number Diff line number Diff line change
Expand Up @@ -893,60 +893,27 @@ const styles = {
flex: 1,
},

signInPageLogo: {
height: variables.componentSizeLarge,
marginBottom: 24,
},

signInPageInner: {
marginLeft: 'auto',
marginRight: 'auto',
height: '100%',
width: '100%',
},

signInPageInnerNative: {
width: '100%',
signInPageContentTopSpacer: {
maxHeight: 132,
minHeight: 24,
},

signInPageHeroHeading: {
fontFamily: fontFamily.GTA,
fontWeight: fontWeightBold,
fontSize: variables.fontSizeHero,
color: themeColors.appBG,
lineHeight: variables.lineHeightHero,
},

signInPageHeroDescription: {
fontFamily: fontFamily.GTA,
fontSize: variables.fontSizeNormal,
color: themeColors.appBG,
},

signInPageFormContainer: {
maxWidth: 295,
width: '100%',
},

signInPageNarrowContentContainer: {
maxWidth: 335,
},

signInPageNarrowContentMargin: {
marginTop: '40%',
},

signInPageWideLeftContainer: {
width: 375,
maxWidth: 375,
},

signInPageWideLeftContentMargin: {
marginTop: '44.5%',
signInPageLeftContainerMediumScreen: {
maxWidth: 400,
marginLeft: 'auto',
marginRight: 'auto',
Copy link
Contributor

Choose a reason for hiding this comment

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

+        marginLeft: 'auto',
+        marginRight: 'auto',

What is this change for?
Because of this, horizontal padding is not consistent between email page and password page
Simulator Screen Shot - iPhone SE (3rd generation) - 2022-12-01 at 12 08 54
Simulator Screen Shot - iPhone SE (3rd generation) - 2022-12-01 at 12 09 03
tested on iPhone SE
you can see button width are different

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh, this stuff is so finicky.

The reason I added the auto-margins is:

  1. Shawn requested to have the form use a max-width
  2. Setting a maxWidth works for the form, but for medium-sized screens, the form is aligned on the left side of the container and looks bad. The form needs to be horizontally centered in the container.
  3. My first thought was to use alignSelfCenter, and that works. BUT at small screen widths, that style also centers the content vertically 😡 and adds a lot of unintended margin to the top and bottom
  4. So I went with the auto margin route, which makes the form horizontally centered AND doesn't mess up any vertical margins

I have fiddled with it again today to remove the margin auto and get the alignSelfCenter working.

I narrowed the problem you saw with iPhone SE down to the props.welcomeText. It seems the difference in text was causing the width of the container to change size... so I have restricted that text to 300px wide always.

Updated!

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you (or a reviewer) share a quick video of the sign in page going from small to wide? Just want to check and see how the responsive styles are working, thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here you go:

2022-12-01_12-50-20.mp4

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, looks good to me, thanks for making the video!

},

signInPageWideHeroContent: {
maxWidth: 400,
signInPageLeftContainer: {
paddingLeft: 40,
paddingRight: 40,
},

changeExpensifyLoginLinkContainer: {
Expand Down
8 changes: 4 additions & 4 deletions src/styles/utilities/flex.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,6 @@ export default {
flexDirection: 'column',
},

flexColumnReverse: {
flexDirection: 'column-reverse',
},

justifyContentCenter: {
justifyContent: 'center',
},
Expand Down Expand Up @@ -108,6 +104,10 @@ export default {
flexGrow: 1,
},

flexGrow2: {
flexGrow: 2,
},

flexGrow4: {
flexGrow: 4,
},
Expand Down
1 change: 0 additions & 1 deletion src/styles/variables.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ export default {
tooltipzIndex: 10050,
gutterWidth: 16,
popoverMenuShadow: '0px 4px 12px 0px rgba(0, 0, 0, 0.06)',
minHeightToShowGraphics: 854, // Login form layout breaks below this height due to insufficient space to show the form and graphics
optionRowHeight: 64,
optionRowHeightCompact: 52,
optionsListSectionHeaderHeight: 54,
Expand Down