-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Viewport jumping up #16443
Viewport jumping up #16443
Changes from 12 commits
0713192
81986b1
be4fdf1
b6c8612
0f0628e
c0af64c
12a307a
276e23b
507f683
5a9acb2
873a6e7
4806e12
1be9686
77345f1
278b51c
afb02f1
b90d6c8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -85,6 +85,8 @@ class ScreenWrapper extends React.Component { | |
} | ||
|
||
render() { | ||
const maxHeight = this.props.shouldEnableMaxHeight ? this.props.windowHeight : null; | ||
|
||
return ( | ||
<SafeAreaConsumer> | ||
{({ | ||
|
@@ -109,7 +111,7 @@ class ScreenWrapper extends React.Component { | |
paddingStyle, | ||
]} | ||
> | ||
<KeyboardAvoidingView style={[styles.w100, styles.h100, {maxHeight: this.props.windowHeight}]} behavior={this.props.keyboardAvoidingViewBehavior}> | ||
<KeyboardAvoidingView style={[styles.w100, styles.h100, {maxHeight}]} behavior={this.props.keyboardAvoidingViewBehavior}> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why don't we depend on The I know that this PR does not introduce this code, but it does the biggest changes around it in a while, so if the original implementation was created in a way I'd describe as "confusing", I think we should clean it up here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then, we'd also have a good place to comment on which browsers actually need that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gedu, what do you think about this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No inline styles cc: @thesahindia |
||
<HeaderGap /> | ||
{// If props.children is a function, call it to provide the insets to the children. | ||
_.isFunction(this.props.children) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
import PropTypes from 'prop-types'; | ||
import * as DeviceCapabilities from '../../libs/DeviceCapabilities'; | ||
|
||
const propTypes = { | ||
/** Array of additional styles to add */ | ||
|
@@ -31,6 +32,9 @@ const propTypes = { | |
|
||
/** Whether to dismiss keyboard before leaving a screen */ | ||
shouldDismissKeyboardBeforeClose: PropTypes.bool, | ||
|
||
/** Whether to use the maxHeight (true) or use the 100% of the height (false) */ | ||
shouldEnableMaxHeight: PropTypes.bool, | ||
}; | ||
|
||
const defaultProps = { | ||
|
@@ -41,6 +45,7 @@ const defaultProps = { | |
onEntryTransitionEnd: () => {}, | ||
modal: {}, | ||
keyboardAvoidingViewBehavior: 'padding', | ||
shouldEnableMaxHeight: !DeviceCapabilities.canUseTouchScreen(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gedu, why aren't we using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because the issue was reported mainly on web mobile, so I wanna leave the behavior for web as is just in case, but I think it can be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. We should use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated |
||
}; | ||
|
||
export {propTypes, defaultProps}; |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -29,6 +29,7 @@ import {withNetwork} from '../../components/OnyxProvider'; | |||||
import reportPropTypes from '../reportPropTypes'; | ||||||
import * as ReportUtils from '../../libs/ReportUtils'; | ||||||
import * as ReportScrollManager from '../../libs/ReportScrollManager'; | ||||||
import * as DeviceCapabilities from '../../libs/DeviceCapabilities'; | ||||||
|
||||||
/** | ||||||
* A modal used for requesting money, splitting bills or sending money. | ||||||
|
@@ -343,8 +344,10 @@ const MoneyRequestModal = (props) => { | |||||
const reportID = lodashGet(props, 'route.params.reportID', ''); | ||||||
const shouldShowBackButton = currentStepIndex > 0; | ||||||
const modalHeader = <ModalHeader title={titleForStep} shouldShowBackButton={shouldShowBackButton} onBackButtonPress={navigateToPreviousStep} />; | ||||||
const enableMaxHeight = DeviceCapabilities.canUseTouchScreen() && currentStep !== Steps.IOUAmount; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
In #16919 we are moving "what's it for" field to a different page so we only need enable max height at the participant page |
||||||
|
||||||
return ( | ||||||
<ScreenWrapper includeSafeAreaPaddingBottom={false}> | ||||||
<ScreenWrapper includeSafeAreaPaddingBottom={false} shouldEnableMaxHeight={enableMaxHeight}> | ||||||
{({didScreenTransitionEnd, safeAreaPaddingBottomStyle}) => ( | ||||||
<> | ||||||
<View style={[styles.pRelative, styles.flex1]}> | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NAB: should we use
undefined
instead ofnull
here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, is better to use
undefined
to remove styles, thanks