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

allow Request money 'Description' to accept multiline #21664

Merged
merged 18 commits into from
Aug 28, 2023
Merged
Show file tree
Hide file tree
Changes from 15 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/DisplayNames/DisplayNamesWithTooltip.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ function DisplayNamesWithToolTip(props) {
return (
// Tokenization of string only support prop numberOfLines on Web
<Text
style={[...props.textStyles, styles.pRelative]}
style={[...props.textStyles, styles.pRelative, props.numberOfLines === 1 ? styles.noWrap : {}]}
Copy link
Contributor

Choose a reason for hiding this comment

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

styles.noWrap is not needed here #32555 (comment)

numberOfLines={props.numberOfLines || undefined}
ref={(el) => (containerRef.current = el)}
>
Expand Down
3 changes: 2 additions & 1 deletion src/components/MoneyRequestConfirmationList.js
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,7 @@ function MoneyRequestConfirmationList(props) {
onConfirmSelection={confirm}
selectedOptions={selectedOptions}
canSelectMultipleOptions={canModifyParticipants}
disableArrowKeysActions={!canModifyParticipants}
disableArrowKeysActions={props.disableArrowKeysActions || !canModifyParticipants}
ahmedGaber93 marked this conversation as resolved.
Show resolved Hide resolved
boldStyle
showTitleTooltip
shouldTextInputAppearBelowOptions
Expand Down Expand Up @@ -357,6 +357,7 @@ function MoneyRequestConfirmationList(props) {
style={[styles.moneyRequestMenuItem, styles.mb2]}
titleStyle={styles.flex1}
disabled={didConfirm || props.isReadOnly}
numberOfLinesTitle={2}
/>
{!showAllFields && (
<View style={[styles.flexRow, styles.justifyContentBetween, styles.mh3, styles.alignItemsCenter]}>
Expand Down
92 changes: 55 additions & 37 deletions src/components/OptionsSelector/BaseOptionsSelector.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,41 +64,22 @@ class BaseOptionsSelector extends Component {
}

componentDidMount() {
const enterConfig = CONST.KEYBOARD_SHORTCUTS.ENTER;
this.unsubscribeEnter = KeyboardShortcut.subscribe(
enterConfig.shortcutKey,
this.selectFocusedOption,
enterConfig.descriptionKey,
enterConfig.modifiers,
true,
() => !this.state.allOptions[this.state.focusedIndex],
);

const CTRLEnterConfig = CONST.KEYBOARD_SHORTCUTS.CTRL_ENTER;
this.unsubscribeCTRLEnter = KeyboardShortcut.subscribe(
CTRLEnterConfig.shortcutKey,
() => {
if (this.props.canSelectMultipleOptions) {
this.props.onConfirmSelection();
return;
}

const focusedOption = this.state.allOptions[this.state.focusedIndex];
if (!focusedOption) {
return;
}

this.selectRow(focusedOption);
},
CTRLEnterConfig.descriptionKey,
CTRLEnterConfig.modifiers,
true,
);
if (!this.props.disableArrowKeysActions) {
ahmedGaber93 marked this conversation as resolved.
Show resolved Hide resolved
this.subscribeToKeyboardShortcut();
}

this.scrollToIndex(this.props.selectedOptions.length ? 0 : this.state.focusedIndex, false);
}

componentDidUpdate(prevProps) {
if (prevProps.disableArrowKeysActions !== this.props.disableArrowKeysActions) {
if (this.props.disableArrowKeysActions) {
this.unSubscribeToKeyboardShortcut();
} else {
this.subscribeToKeyboardShortcut();
}
}

if (this.textInput && this.props.autoFocus && !prevProps.isFocused && this.props.isFocused) {
InteractionManager.runAfterInteractions(() => {
// If we automatically focus on a text input when mounting a component,
Expand Down Expand Up @@ -148,13 +129,7 @@ class BaseOptionsSelector extends Component {
clearTimeout(this.focusTimeout);
}

if (this.unsubscribeEnter) {
this.unsubscribeEnter();
}

if (this.unsubscribeCTRLEnter) {
this.unsubscribeCTRLEnter();
}
this.unSubscribeToKeyboardShortcut();
}

/**
Expand All @@ -179,6 +154,49 @@ class BaseOptionsSelector extends Component {
return defaultIndex;
}

subscribeToKeyboardShortcut() {
const enterConfig = CONST.KEYBOARD_SHORTCUTS.ENTER;
this.unsubscribeEnter = KeyboardShortcut.subscribe(
enterConfig.shortcutKey,
this.selectFocusedOption,
enterConfig.descriptionKey,
enterConfig.modifiers,
true,
() => !this.state.allOptions[this.state.focusedIndex],
);

const CTRLEnterConfig = CONST.KEYBOARD_SHORTCUTS.CTRL_ENTER;
this.unsubscribeCTRLEnter = KeyboardShortcut.subscribe(
CTRLEnterConfig.shortcutKey,
() => {
if (this.props.canSelectMultipleOptions) {
this.props.onConfirmSelection();
return;
}

const focusedOption = this.state.allOptions[this.state.focusedIndex];
if (!focusedOption) {
return;
}

this.selectRow(focusedOption);
},
CTRLEnterConfig.descriptionKey,
CTRLEnterConfig.modifiers,
true,
);
}

unSubscribeToKeyboardShortcut() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
unSubscribeToKeyboardShortcut() {
unSubscribeFromKeyboardShortcut() {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed

if (this.unsubscribeEnter) {
this.unsubscribeEnter();
}

if (this.unsubscribeCTRLEnter) {
this.unsubscribeCTRLEnter();
}
}

selectFocusedOption() {
const focusedOption = this.state.allOptions[this.state.focusedIndex];

Expand Down
2 changes: 1 addition & 1 deletion src/components/ReportActionItem/MoneyRequestPreview.js
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ function MoneyRequestPreview(props) {
</View>
{!props.isBillSplit && !_.isEmpty(requestMerchant) && (
<View style={[styles.flexRow]}>
<Text style={[styles.textLabelSupporting, styles.mb1, styles.lh16]}>{requestMerchant}</Text>
<Text style={[styles.textLabelSupporting, styles.mb1, styles.lh16, styles.breakWord]}>{requestMerchant}</Text>
</View>
)}
<View style={[styles.flexRow]}>
Expand Down
2 changes: 1 addition & 1 deletion src/libs/actions/IOU.js
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ function buildOnyxDataForMoneyRequest(
key: `${ONYXKEYS.COLLECTION.REPORT}${iouReport.reportID}`,
value: {
...iouReport,
lastMessageText: iouAction.message[0].text,
lastMessageText: ReportUtils.formatReportLastMessageText(iouAction.message[0].text),
Copy link
Contributor

Choose a reason for hiding this comment

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

Apply this to Split bill as well

Screenshot 2023-08-25 at 12 50 13 PM

Copy link
Contributor

@aimane-chnaif aimane-chnaif Aug 25, 2023

Choose a reason for hiding this comment

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

Actually I think this can be separate issue to fix inconsistency between optimistic data and backend data for last message text with multiline description.
Can't we just prevent wrap 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.

Can't we just prevent wrap here?

Are you mean remove new lines "\n" from message text?
If you mean that the lastMessageText will not be inconsistent with backend, because backend return only first line of message text.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we just prevent wrap here?

I meant to keep that optimistic value as is now but just fix display issue of wrapped

Copy link
Contributor Author

@ahmedGaber93 ahmedGaber93 Aug 25, 2023

Choose a reason for hiding this comment

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

the wrap happened here because numberOfLines={1} does not work as expected when <Text/> contains \n and whiteSpace: 'pre' just not break the long lines but not affected on \n

For that, we can fix it by replace styles.pre with styles.noWrap here

const alternateTextStyle = StyleUtils.combineStyles(
props.viewMode === CONST.OPTION_MODE.COMPACT
? [textStyle, styles.optionAlternateText, styles.pre, styles.textLabelSupporting, styles.optionAlternateTextCompact, styles.ml2]
: [textStyle, styles.optionAlternateText, styles.pre, styles.textLabelSupporting],
props.style,
);

This will remove the new lines "\n" from message text and fix the wrap issue, but inconsistency with backend will still issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Workaround cannot be a quick fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't find a quick fix right now. What is the next step now?

  • use styles.noWrap and fix wrap issue partially with inconsistent result on iOS.
  • do nothing and keep wrap issue separately?

Copy link
Contributor

Choose a reason for hiding this comment

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

do nothing and keep wrap issue separately?

We can't do this because it looks weird and our PR might need to be reverted.

use styles.noWrap and fix wrap issue partially with inconsistent result on iOS.

This is still concerned but we would consider this not a deploy blocker I believe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I update the code to use styles.noWrap

Copy link
Contributor

Choose a reason for hiding this comment

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

styles.noWrap caused regression - #27586

lastMessageHtml: iouAction.message[0].html,
...(isNewIOUReport ? {pendingFields: {createChat: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD}} : {}),
},
Expand Down
8 changes: 7 additions & 1 deletion src/pages/iou/MoneyRequestDescriptionPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import * as IOU from '../../libs/actions/IOU';
import optionPropTypes from '../../components/optionPropTypes';
import CONST from '../../CONST';
import useLocalize from '../../hooks/useLocalize';
import focusAndUpdateMultilineInputRange from '../../libs/focusAndUpdateMultilineInputRange';
import * as Browser from '../../libs/Browser';

const propTypes = {
/** Onyx Props */
Expand Down Expand Up @@ -88,7 +90,7 @@ function MoneyRequestDescriptionPage({iou, route}) {
<ScreenWrapper
includeSafeAreaPaddingBottom={false}
shouldEnableMaxHeight
onEntryTransitionEnd={() => inputRef.current && inputRef.current.focus()}
onEntryTransitionEnd={() => focusAndUpdateMultilineInputRange(inputRef.current)}
>
<HeaderWithBackButton
title={translate('common.description')}
Expand All @@ -110,6 +112,10 @@ function MoneyRequestDescriptionPage({iou, route}) {
accessibilityLabel={translate('moneyRequestConfirmationList.whatsItFor')}
accessibilityRole={CONST.ACCESSIBILITY_ROLE.TEXT}
ref={(el) => (inputRef.current = el)}
autoGrowHeight
containerStyles={[styles.autoGrowHeightMultilineInput]}
textAlignVertical="top"
submitOnEnter={!Browser.isMobile()}
/>
</View>
</Form>
Expand Down
4 changes: 4 additions & 0 deletions src/pages/iou/steps/MoneyRequestConfirmPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import PropTypes from 'prop-types';
import {withOnyx} from 'react-native-onyx';
import _ from 'underscore';
import lodashGet from 'lodash/get';
import {useIsFocused} from '@react-navigation/native';
import MoneyRequestConfirmationList from '../../../components/MoneyRequestConfirmationList';
import CONST from '../../../CONST';
import ScreenWrapper from '../../../components/ScreenWrapper';
Expand Down Expand Up @@ -79,6 +80,8 @@ function MoneyRequestConfirmPage(props) {
[props.iou.participants, props.personalDetails],
);

const isFocused = useIsFocused();

useEffect(() => {
const policyExpenseChat = _.find(participants, (participant) => participant.isPolicyExpenseChat);
if (policyExpenseChat) {
Expand Down Expand Up @@ -266,6 +269,7 @@ function MoneyRequestConfirmPage(props) {
bankAccountRoute={ReportUtils.getBankAccountRoute(props.report)}
iouMerchant={props.iou.merchant}
iouCreated={props.iou.created}
disableArrowKeysActions={!isFocused}
/>
</View>
)}
Expand Down