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

Viewport jumping up #16443

Merged
merged 17 commits into from
Apr 21, 2023
Merged

Viewport jumping up #16443

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
0713192
New locking height prop
gedu Mar 22, 2023
81986b1
Cleanups
gedu Mar 23, 2023
be4fdf1
add lockHeight to Profile and SideBar Pages
gedu Mar 27, 2023
b6c8612
locking height to ReportScreen for messages
gedu Mar 28, 2023
0f0628e
Merge branch 'main' into edu/15270_viewport_jumping_up
gedu Mar 28, 2023
c0af64c
lockheight for add payment screen
gedu Mar 31, 2023
12a307a
Merge branch 'main' into edu/15270_viewport_jumping_up
gedu Apr 5, 2023
276e23b
Updated max height handling
gedu Apr 5, 2023
507f683
deleted again IOUModal
gedu Apr 5, 2023
5a9acb2
fixing comments
gedu Apr 7, 2023
873a6e7
Renamed enableMaxHeight to shouldEnableMaxHeight
gedu Apr 11, 2023
4806e12
removed shouldEnabledMaxHeight here isn't necessary because now is fa…
gedu Apr 11, 2023
1be9686
setting undefined to the style to remove it
gedu Apr 11, 2023
77345f1
let the bottom button to move up
gedu Apr 12, 2023
278b51c
Merge branch 'main' into edu/15270_viewport_jumping_up
gedu Apr 13, 2023
afb02f1
Merge branch 'main' into edu/15270_viewport_jumping_up
gedu Apr 17, 2023
b90d6c8
Changed shouldEnableMaxHeight to be false as default
gedu Apr 18, 2023
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
4 changes: 3 additions & 1 deletion src/components/ScreenWrapper/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,8 @@ class ScreenWrapper extends React.Component {
}

render() {
const maxHeight = this.props.shouldEnableMaxHeight ? this.props.windowHeight : undefined;

return (
<SafeAreaConsumer>
{({
Expand All @@ -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}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we depend on Dimensions (windowHeight) inside KeyboardAvoidingView/index.web.js and apply maxHeight there? That would be a clean symmetry to the index.ios.js implementation and would reflect the general contract of the built-in KeyboardAvoidingView.

The shouldEnableMaxHeight would need to be forwarded to the in-house KeyboardAvoidingView, but would be accessed only on the Web implementation, which would improve clarity. Because the maxHeight thing is only for the Web, right?

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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

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

@gedu, what do you think about this?

Copy link
Member

Choose a reason for hiding this comment

The 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)
Expand Down
5 changes: 5 additions & 0 deletions src/components/ScreenWrapper/propTypes.js
View file
Open in desktop
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';
import {windowDimensionsPropTypes} from '../withWindowDimensions';

const propTypes = {
Expand Down Expand Up @@ -33,6 +34,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,

...windowDimensionsPropTypes,
};

Expand All @@ -44,6 +48,7 @@ const defaultProps = {
onEntryTransitionEnd: () => {},
modal: {},
keyboardAvoidingViewBehavior: 'padding',
shouldEnableMaxHeight: !DeviceCapabilities.canUseTouchScreen(),
Copy link
Member

Choose a reason for hiding this comment

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

@gedu, why aren't we using false here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 false, should I change it?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. We should use false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

};

export {propTypes, defaultProps};
2 changes: 1 addition & 1 deletion src/pages/NewChatPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,7 +233,7 @@ class NewChatPage extends Component {
maxParticipantsReached,
);
return (
<ScreenWrapper includeSafeAreaPaddingBottom={false}>
<ScreenWrapper includeSafeAreaPaddingBottom={false} shouldEnableMaxHeight>
{({didScreenTransitionEnd, safeAreaPaddingBottomStyle}) => (
<>
<HeaderWithCloseButton
Expand Down
2 changes: 1 addition & 1 deletion src/pages/iou/MoneyRequestDescriptionPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ class MoneyRequestDescriptionPage extends Component {

render() {
return (
<ScreenWrapper includeSafeAreaPaddingBottom={false}>
<ScreenWrapper includeSafeAreaPaddingBottom={false} shouldEnableMaxHeight>
<HeaderWithCloseButton
title={this.props.translate('common.description')}
shouldShowBackButton
Expand Down
5 changes: 4 additions & 1 deletion src/pages/iou/MoneyRequestModal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,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.
Expand Down Expand Up @@ -367,8 +368,10 @@ const MoneyRequestModal = (props) => {
/>
);
const amountButtonText = isEditingAmountAfterConfirm ? props.translate('common.save') : props.translate('common.next');
const enableMaxHeight = DeviceCapabilities.canUseTouchScreen() && currentStep === Steps.MoneyRequestParticipants;

return (
<ScreenWrapper includeSafeAreaPaddingBottom={false}>
<ScreenWrapper includeSafeAreaPaddingBottom={false} shouldEnableMaxHeight={enableMaxHeight}>
{({didScreenTransitionEnd, safeAreaPaddingBottomStyle}) => (
<>
<View style={[styles.pRelative, styles.flex1]}>
Expand Down
2 changes: 1 addition & 1 deletion src/pages/workspace/WorkspaceInvitePage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,7 @@ class WorkspaceInvitePage extends React.Component {
const policyName = lodashGet(this.props.policy, 'name');

return (
<ScreenWrapper>
<ScreenWrapper shouldEnableMaxHeight>
{({didScreenTransitionEnd}) => (
<FullPageNotFoundView
shouldShow={_.isEmpty(this.props.policy)}
Expand Down