Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
5 changes: 5 additions & 0 deletions src/components/MoneyRequestConfirmationList.js
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,9 @@ const propTypes = {
/** File source of the receipt */
receiptSource: PropTypes.string,

/** List styles for OptionsSelector */
listStyles: PropTypes.oneOfType([PropTypes.arrayOf(PropTypes.object), PropTypes.object]),

/* Onyx Props */
/** Collection of categories attached to a policy */
policyCategories: PropTypes.objectOf(categoryPropTypes),
Expand All @@ -120,6 +123,7 @@ const defaultProps = {
...withCurrentUserPersonalDetailsDefaultProps,
receiptPath: '',
receiptSource: '',
listStyles: [],
policyCategories: {},
};

Expand Down Expand Up @@ -344,6 +348,7 @@ function MoneyRequestConfirmationList(props) {
shouldUseStyleForChildren={false}
optionHoveredStyle={canModifyParticipants ? styles.hoveredComponentBG : {}}
footerContent={footerContent}
listStyles={props.listStyles}
>
{!_.isEmpty(props.receiptPath) ? (
<Image
Expand Down
7 changes: 7 additions & 0 deletions src/components/OptionsList/BaseOptionsList.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,20 +21,25 @@ const propTypes = {
/** Callback executed on scroll. Only used for web/desktop component */
onScroll: PropTypes.func,

/** List styles for SectionList */
listStyles: PropTypes.oneOfType([PropTypes.arrayOf(PropTypes.object), PropTypes.object]),

...optionsListPropTypes,
};

const defaultProps = {
keyboardDismissMode: 'none',
onScrollBeginDrag: () => {},
onScroll: () => {},
listStyles: [],
...optionsListDefaultProps,
};

function BaseOptionsList({
keyboardDismissMode,
onScrollBeginDrag,
onScroll,
listStyles,
focusedIndex,
selectedOptions,
headerMessage,
Expand Down Expand Up @@ -219,6 +224,8 @@ function BaseOptionsList({
) : null}
<SectionList
ref={innerRef}
nestedScrollEnabled

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

  1. nestedScrollEnabled is true by default on iOS
  2. nestedScrollEnabled is false by default on Android, and it was added in API 21 which is exactly the lowest we support right now.
  3. nestedScrollEnabled should have no effect on browsers.

Leaving it to true as default here would put both iOS and Android on the same line, and it should have no negative effect in the application for any platform. I've looked over places where this component is used, and couldn't find any new bugs/different behaviors.

style={listStyles}
indicatorStyle="white"
keyboardShouldPersistTaps="always"
keyboardDismissMode={keyboardDismissMode}
Expand Down
5 changes: 5 additions & 0 deletions src/components/OptionsSelector/BaseOptionsSelector.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,9 @@ const propTypes = {
/** List container styles for OptionsList */
listContainerStyles: PropTypes.oneOfType([PropTypes.arrayOf(PropTypes.object), PropTypes.object]),

/** List styles for OptionsList */
listStyles: PropTypes.oneOfType([PropTypes.arrayOf(PropTypes.object), PropTypes.object]),

...optionsSelectorPropTypes,
...withLocalizePropTypes,
...withNavigationFocusPropTypes,
Expand All @@ -40,6 +43,7 @@ const defaultProps = {
safeAreaPaddingBottomStyle: {},
contentContainerStyles: [],
listContainerStyles: [styles.flex1],
listStyles: [],
...optionsSelectorDefaultProps,
};

Expand Down Expand Up @@ -373,6 +377,7 @@ class BaseOptionsSelector extends Component {
}}
contentContainerStyles={[safeAreaPaddingBottomStyle, ...this.props.contentContainerStyles]}
listContainerStyles={this.props.listContainerStyles}
listStyles={this.props.listStyles}
isLoading={!this.props.shouldShowOptions}
showScrollIndicator={this.props.showScrollIndicator}
/>
Expand Down
6 changes: 3 additions & 3 deletions src/pages/iou/steps/MoneyRequestAmountForm.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import React, {useEffect, useState, useCallback, useRef} from 'react';
import {View} from 'react-native';
import {ScrollView, View} from 'react-native';
import PropTypes from 'prop-types';
import lodashGet from 'lodash/get';
import _ from 'underscore';
Expand Down Expand Up @@ -200,7 +200,7 @@ function MoneyRequestAmountForm({amount, currency, isEditing, forwardedRef, onCu
const buttonText = isEditing ? translate('common.save') : translate('common.next');

return (
<>
<ScrollView contentContainerStyle={styles.flexGrow1}>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We need contentContainerStyle={styles.flexGrow1} because we want this page to use the whole vertically available space even if the space is larger than the page. (same behavior as seen on current staging, removing this would make the page not place the custom keyboard at the bottom of the screen).

<View
nativeID={AMOUNT_VIEW_ID}
onMouseDown={(event) => onMouseDown(event, [AMOUNT_VIEW_ID])}
Expand Down Expand Up @@ -253,7 +253,7 @@ function MoneyRequestAmountForm({amount, currency, isEditing, forwardedRef, onCu
text={buttonText}
/>
</View>
</>
</ScrollView>
);
}

Expand Down
82 changes: 49 additions & 33 deletions src/pages/iou/steps/MoneyRequestConfirmPage.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import React, {useCallback, useEffect, useMemo, useRef} from 'react';
import {View} from 'react-native';
import {ScrollView, View} from 'react-native';
import PropTypes from 'prop-types';
import {withOnyx} from 'react-native-onyx';
import _ from 'underscore';
Expand All @@ -22,6 +22,8 @@ import reportPropTypes from '../../reportPropTypes';
import personalDetailsPropType from '../../personalDetailsPropType';
import * as FileUtils from '../../../libs/fileDownload/FileUtils';
import * as Policy from '../../../libs/actions/Policy';
import useWindowDimensions from '../../../hooks/useWindowDimensions';
import * as StyleUtils from '../../../styles/StyleUtils';
import {iouPropTypes, iouDefaultProps} from '../propTypes';

const propTypes = {
Expand Down Expand Up @@ -56,6 +58,7 @@ const defaultProps = {
};

function MoneyRequestConfirmPage(props) {
const {windowHeight} = useWindowDimensions();
const prevMoneyRequestId = useRef(props.iou.id);
const iouType = useRef(lodashGet(props.route, 'params.iouType', ''));
const reportID = useRef(lodashGet(props.route, 'params.reportID', ''));
Expand Down Expand Up @@ -223,38 +226,51 @@ function MoneyRequestConfirmPage(props) {
title={props.translate('iou.cash')}
onBackButtonPress={navigateBack}
/>
<MoneyRequestConfirmationList
hasMultipleParticipants={iouType.current === CONST.IOU.MONEY_REQUEST_TYPE.SPLIT}
selectedParticipants={participants}
iouAmount={props.iou.amount}
iouComment={props.iou.comment}
iouCurrencyCode={props.iou.currency}
onConfirm={createTransaction}
onSendMoney={sendMoney}
onSelectParticipant={(option) => {
const newParticipants = _.map(props.iou.participants, (participant) => {
if (participant.accountID === option.accountID) {
return {...participant, selected: !participant.selected};
}
return participant;
});
IOU.setMoneyRequestParticipants(newParticipants);
}}
receiptPath={props.iou.receiptPath}
receiptSource={props.iou.receiptSource}
iouType={iouType.current}
reportID={reportID.current}
// The participants can only be modified when the action is initiated from directly within a group chat and not the floating-action-button.
// This is because when there is a group of people, say they are on a trip, and you have some shared expenses with some of the people,
// but not all of them (maybe someone skipped out on dinner). Then it's nice to be able to select/deselect people from the group chat bill
// split rather than forcing the user to create a new group, just for that expense. The reportID is empty, when the action was initiated from
// the floating-action-button (since it is something that exists outside the context of a report).
canModifyParticipants={!_.isEmpty(reportID.current)}
policyID={props.report.policyID}
bankAccountRoute={ReportUtils.getBankAccountRoute(props.report)}
iouMerchant={props.iou.merchant}
iouCreated={props.iou.created}
/>
{/*
* The MoneyRequestConfirmationList component uses a SectionList which uses a VirtualizedList internally.
* VirtualizedList cannot be directly nested within ScrollViews of the same orientation.
* To work around this, we wrap the MoneyRequestConfirmationList component with a horizontal ScrollView.
*/}
<ScrollView>
<ScrollView
horizontal
contentContainerStyle={[styles.flex1, styles.flexColumn]}
>
Comment on lines +229 to +238

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't see the problem or the need for the double ScrollView. The VirtualizedList does not seem to be directly nested. Can you please double checked whether this is needed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@s77rt It is needed because MoneyRequestConfirmationList uses SectionList. From the docs:

This is a convenience wrapper around VirtualizedList, and thus inherits its props (as well as those of ScrollView) that aren't explicitly listed here...

Running the app on iOS/Android will throw a runtime error and may also break up some functionality as seen in this video:

mobile.mp4

@s77rt s77rt Sep 4, 2023 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah I see you are testing with a vertical ScrollView. What I meant is to use one horizontal ScrollView. Can we use that?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

My bad here. We need a vertical scrollview. Thanks for your quick responses!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, we can't. The horizontal prop of the child ScrollView must be removed together with the parent ScrollView, otherwise the whole page won't be scrollable.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@s77rt

Thanks for your quick responses!

No problem! Sorry for the delay, I was out of town over the weekend and just got home. I would have replied sooner if I could, sorry again!

<MoneyRequestConfirmationList
hasMultipleParticipants={iouType.current === CONST.IOU.MONEY_REQUEST_TYPE.SPLIT}
selectedParticipants={participants}
iouAmount={props.iou.amount}
iouComment={props.iou.comment}
iouCurrencyCode={props.iou.currency}
onConfirm={createTransaction}
onSendMoney={sendMoney}
onSelectParticipant={(option) => {
const newParticipants = _.map(props.iou.participants, (participant) => {
if (participant.accountID === option.accountID) {
return {...participant, selected: !participant.selected};
}
return participant;
});
IOU.setMoneyRequestParticipants(newParticipants);
}}
receiptPath={props.iou.receiptPath}
receiptSource={props.iou.receiptSource}
iouType={iouType.current}
reportID={reportID.current}
// The participants can only be modified when the action is initiated from directly within a group chat and not the floating-action-button.
// This is because when there is a group of people, say they are on a trip, and you have some shared expenses with some of the people,
// but not all of them (maybe someone skipped out on dinner). Then it's nice to be able to select/deselect people from the group chat bill
// split rather than forcing the user to create a new group, just for that expense. The reportID is empty, when the action was initiated from
// the floating-action-button (since it is something that exists outside the context of a report).
canModifyParticipants={!_.isEmpty(reportID.current)}
policyID={props.report.policyID}
bankAccountRoute={ReportUtils.getBankAccountRoute(props.report)}
iouMerchant={props.iou.merchant}
iouCreated={props.iou.created}
listStyles={[StyleUtils.getMaximumHeight(windowHeight / 3)]}
/>
</ScrollView>
</ScrollView>
</View>
)}
</ScreenWrapper>
Expand Down
12 changes: 12 additions & 0 deletions src/styles/StyleUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -833,6 +833,17 @@ function getMinimumHeight(minHeight) {
};
}

/**
* Get maximum height as style
* @param {Number} maxHeight
* @returns {Object}
*/
function getMaximumHeight(maxHeight) {
return {
maxHeight,
};
}

/**
* Get maximum width as style
* @param {Number} maxWidth
Expand Down Expand Up @@ -1370,6 +1381,7 @@ export {
hasSafeAreas,
getHeight,
getMinimumHeight,
getMaximumHeight,
getMaximumWidth,
fade,
getHorizontalStackedAvatarBorderStyle,
Expand Down