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

Move the cancel and save changes to inside the compose box #17633

Merged
merged 6 commits into from
May 4, 2023
Merged

Move the cancel and save changes to inside the compose box #17633

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1cece21
Move buttons into edit composer
terrysahaidak Apr 18, 2023
a9ae3f2
Follow same spacing on android as ReportActionComposer
terrysahaidak Apr 19, 2023
14e773c
clean-up
terrysahaidak Apr 19, 2023
be5ab37
Better formatting
terrysahaidak Apr 21, 2023
e87637b
Improve comments
terrysahaidak Apr 21, 2023
3dc4557
Fix ExceededCommentLength and address review feedback
terrysahaidak May 4, 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
67 changes: 46 additions & 21 deletions src/pages/home/report/ReportActionItem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ import * as Expensicons from '../../../components/Icon/Expensicons';
import Text from '../../../components/Text';
import DisplayNames from '../../../components/DisplayNames';
import personalDetailsPropType from '../../personalDetailsPropType';
import ReportActionItemDraft from './ReportActionItemDraft';

const propTypes = {
/** Report for this action */
Expand Down Expand Up @@ -239,15 +240,55 @@ class ReportActionItem extends Component {
<>
{children}
{hasReactions && (
<ReportActionItemReactions
reactions={reactions}
toggleReaction={this.toggleReaction}
/>
<View style={this.props.draftMessage ? styles.chatItemReactionsDraftRight : null}>
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
<View style={this.props.draftMessage ? styles.chatItemReactionsDraftRight : null}>
<View style={this.props.draftMessage ? styles.chatItemReactionsDraftRight : {}}>

Avoid apply null to style.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checked the repo's code and there is the usage of: null, {}, and undefined.
I don't have a preference so I'm ok with {} in this case.

<ReportActionItemReactions
reactions={reactions}
toggleReaction={this.toggleReaction}
/>
</View>
)}
</>
);
}

/**
* Get ReportActionItem with a proper wrapper
* @param {Boolean} hovered whether the ReportActionItem is hovered
* @param {Boolean} isWhisper whether the ReportActionItem is a whisper
* @returns {Object} report action item
*/
renderReportActionItem(hovered, isWhisper) {
const content = this.renderItemContent(hovered || this.state.isContextMenuActive);

if (this.props.draftMessage) {
return (
<ReportActionItemDraft>
{content}
</ReportActionItemDraft>
);
}

if (!this.props.displayAsGroup) {
return (
<ReportActionItemSingle
action={this.props.action}
showHeader={!this.props.draftMessage}
wrapperStyles={[styles.chatItem, isWhisper ? styles.pt1 : {}]}
shouldShowSubscriptAvatar={this.props.shouldShowSubscriptAvatar}
report={this.props.report}
>
{content}
</ReportActionItemSingle>
);
}

return (
<ReportActionItemGrouped wrapperStyles={[styles.chatItem, isWhisper ? styles.pt1 : {}]}>
{content}
</ReportActionItemGrouped>
);
}

render() {
if (this.props.action.actionName === CONST.REPORT.ACTIONS.TYPE.CREATED) {
return <ReportActionItemCreated reportID={this.props.report.reportID} />;
Expand Down Expand Up @@ -335,23 +376,7 @@ class ReportActionItem extends Component {
/>
</View>
)}
{!this.props.displayAsGroup
? (
<ReportActionItemSingle
action={this.props.action}
showHeader={!this.props.draftMessage}
wrapperStyles={[styles.chatItem, isWhisper ? styles.pt1 : {}]}
shouldShowSubscriptAvatar={this.props.shouldShowSubscriptAvatar}
report={this.props.report}
>
{this.renderItemContent(hovered || this.state.isContextMenuActive)}
</ReportActionItemSingle>
)
: (
<ReportActionItemGrouped wrapperStyles={[styles.chatItem, isWhisper ? styles.pt1 : {}]}>
{this.renderItemContent(hovered || this.state.isContextMenuActive)}
</ReportActionItemGrouped>
)}
{this.renderReportActionItem(hovered, isWhisper)}
</OfflineWithFeedback>
</View>
</View>
Expand Down
21 changes: 21 additions & 0 deletions src/pages/home/report/ReportActionItemDraft.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import React from 'react';
import {View} from 'react-native';
import PropTypes from 'prop-types';
import styles from '../../../styles/styles';

const propTypes = {
/** Children view component for this action item */
children: PropTypes.node.isRequired,
};

const ReportActionItemDraft = props => (
<View style={[styles.chatItemDraft]}>
<View style={styles.flex1}>
{props.children}
</View>
</View>
);

ReportActionItemDraft.propTypes = propTypes;
ReportActionItemDraft.displayName = 'ReportActionItemDraft';
export default ReportActionItemDraft;
146 changes: 88 additions & 58 deletions src/pages/home/report/ReportActionItemMessageEdit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,25 +1,32 @@
/* eslint-disable rulesdir/onyx-props-must-have-default */
import lodashGet from 'lodash/get';
import React from 'react';
import {InteractionManager, Keyboard, View} from 'react-native';
import {
InteractionManager, Keyboard, Pressable, TouchableOpacity, View,
} from 'react-native';
import PropTypes from 'prop-types';
import _ from 'underscore';
import ExpensiMark from 'expensify-common/lib/ExpensiMark';
import Str from 'expensify-common/lib/str';
import reportActionPropTypes from './reportActionPropTypes';
import styles from '../../../styles/styles';
import themeColors from '../../../styles/themes/default';
import * as StyleUtils from '../../../styles/StyleUtils';
import Composer from '../../../components/Composer';
import * as Report from '../../../libs/actions/Report';
import * as ReportScrollManager from '../../../libs/ReportScrollManager';
import toggleReportActionComposeView from '../../../libs/toggleReportActionComposeView';
import openReportActionComposeViewWhenClosingMessageEdit from '../../../libs/openReportActionComposeViewWhenClosingMessageEdit';
import Button from '../../../components/Button';
import ReportActionComposeFocusManager from '../../../libs/ReportActionComposeFocusManager';
import compose from '../../../libs/compose';
import EmojiPickerButton from '../../../components/EmojiPicker/EmojiPickerButton';
import Icon from '../../../components/Icon';
import * as Expensicons from '../../../components/Icon/Expensicons';
import Tooltip from '../../../components/Tooltip';
import * as ReportActionContextMenu from './ContextMenu/ReportActionContextMenu';
import * as ReportUtils from '../../../libs/ReportUtils';
import * as EmojiUtils from '../../../libs/EmojiUtils';
import getButtonState from '../../../libs/getButtonState';
import reportPropTypes from '../../reportPropTypes';
import ExceededCommentLength from '../../../components/ExceededCommentLength';
import CONST from '../../../CONST';
Expand Down Expand Up @@ -255,49 +262,71 @@ class ReportActionItemMessageEdit extends React.Component {
render() {
const hasExceededMaxCommentLength = this.state.hasExceededMaxCommentLength;
return (
<View style={styles.chatItemMessage}>
<View style={[styles.chatItemMessage, styles.flexRow]}>
<View
style={[styles.justifyContentEnd]}
>
<Tooltip text={this.props.translate('common.cancel')}>
<Pressable
style={({hovered, pressed}) => ([styles.chatItemSubmitButton, StyleUtils.getButtonBackgroundColorStyle(getButtonState(hovered, pressed))])}
nativeID={this.cancelButtonID}
onPress={this.deleteDraft}
hitSlop={{
top: 3, right: 3, bottom: 3, left: 3,
}}
disabled={hasExceededMaxCommentLength}
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
disabled={hasExceededMaxCommentLength}

I think we should remove this prop as this is a cancel button, and we should be able to cancel the edit regardless of hasExceededMaxCommentLength.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thanks!

>
{({hovered, pressed}) => (
<Icon src={Expensicons.Close} fill={StyleUtils.getIconFillColor(getButtonState(hovered, pressed))} />
)}
</Pressable>
</Tooltip>
</View>
<View
style={[
styles.chatItemComposeBox,
styles.flexRow,
this.state.isFocused ? styles.chatItemComposeBoxFocusedColor : styles.chatItemComposeBoxColor,
styles.flexRow,
styles.flex1,
styles.chatItemComposeBox,
hasExceededMaxCommentLength && styles.borderColorDanger,
]}
>
<Composer
multiline
ref={(el) => {
this.textInput = el;
this.props.forwardedRef(el);
}}
nativeID={this.messageEditInput}
onChangeText={this.updateDraft} // Debounced saveDraftComment
onKeyPress={this.triggerSaveOrCancel}
value={this.state.draft}
maxLines={16} // This is the same that slack has
style={[styles.textInputCompose, styles.flex4, styles.editInputComposeSpacing]}
onFocus={() => {
this.setState({isFocused: true});
ReportScrollManager.scrollToIndex({animated: true, index: this.props.index}, true);
toggleReportActionComposeView(false, this.props.isSmallScreenWidth);
}}
onBlur={(event) => {
this.setState({isFocused: false});
const relatedTargetId = lodashGet(event, 'nativeEvent.relatedTarget.id');

// Return to prevent re-render when save/cancel button is pressed which cancels the onPress event by re-rendering
if (_.contains([this.saveButtonID, this.cancelButtonID, this.emojiButtonID], relatedTargetId)) {
return;
}

if (this.messageEditInput === relatedTargetId) {
return;
}
openReportActionComposeViewWhenClosingMessageEdit(this.props.isSmallScreenWidth);
}}
selection={this.state.selection}
onSelectionChange={this.onSelectionChange}
/>
<View style={styles.textInputComposeSpacing}>
<Composer
multiline
ref={(el) => {
this.textInput = el;
this.props.forwardedRef(el);
}}
nativeID={this.messageEditInput}
onChangeText={this.updateDraft} // Debounced saveDraftComment
onKeyPress={this.triggerSaveOrCancel}
value={this.state.draft}
maxLines={16} // This is the same that slack has
style={[styles.textInputCompose, styles.flex1, styles.bgTransparent]}
onFocus={() => {
this.setState({isFocused: true});
ReportScrollManager.scrollToIndex({animated: true, index: this.props.index}, true);
toggleReportActionComposeView(false, this.props.isSmallScreenWidth);
}}
onBlur={(event) => {
this.setState({isFocused: false});
const relatedTargetId = lodashGet(event, 'nativeEvent.relatedTarget.id');

// Return to prevent re-render when save/cancel button is pressed which cancels the onPress event by re-rendering
if (_.contains([this.saveButtonID, this.cancelButtonID, this.emojiButtonID], relatedTargetId)) {
return;
}

if (this.messageEditInput === relatedTargetId) {
return;
}
openReportActionComposeViewWhenClosingMessageEdit(this.props.isSmallScreenWidth);
}}
selection={this.state.selection}
onSelectionChange={this.onSelectionChange}
/>
</View>
<View style={styles.editChatItemEmojiWrapper}>
<EmojiPickerButton
isDisabled={this.props.shouldDisableEmojiPicker}
Expand All @@ -307,26 +336,27 @@ class ReportActionItemMessageEdit extends React.Component {
/>
</View>

<View style={styles.alignSelfEnd}>
<Tooltip text={this.props.translate('common.saveChanges')}>
<TouchableOpacity
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAIK TouchableOpacity is being deprecated. Can we use Pressable instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Touchable Opacity was used to match exactly the send button we have in the ReportActionComposer.js.

In my opinion, changing it to Pressable would also mean changing it in ReportActionComposer which is already out of the scope of this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I still think that we should change this to a Pressable since TouchableOpacity is being deprecated. I understand if we wanna tackle ReportActionComposer in a followup so we don't add more testing scope to this issue though.

Copy link
Contributor Author

@terrysahaidak terrysahaidak Apr 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

If there's going to be a follow up PR changing ReportActionComposer to use Pressable instead of TouchableOpacity then it makes more sense to me to do both at the same time.

By the way, TouchableOpacity is not deprecated. On reactnative.dev they recommend to use Pressable if you want to have more control:

If you're looking for a more extensive and future-proof way to handle touch-based input, check out the Pressable API.

Also, Pressable is not the same as TouchableOpacity since the latter implements animation out of the box, Pressable doesn't have animation.
Pressabe is designed to be a building block for custom buttons. So the ultimate approach would be to implement our own Touchable Opacity using Pressabe.

style={[styles.chatItemSubmitButton,
(this.state.isCommentEmpty || hasExceededMaxCommentLength) ? undefined : styles.buttonSuccess,
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
(this.state.isCommentEmpty || hasExceededMaxCommentLength) ? undefined : styles.buttonSuccess,
hasExceededMaxCommentLength ? {} : styles.buttonSuccess,
  1. Saving empty message should delete it. I tested that and it's working.
  2. this.state.isCommentEmpty is not defined, it should be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I miss TypeScript.

]}

onPress={this.publishDraft}
hitSlop={{
top: 3, right: 3, bottom: 3, left: 3,
}}
nativeID={this.saveButtonID}
disabled={hasExceededMaxCommentLength}
>
<Icon src={Expensicons.Checkmark} fill={(this.state.isCommentEmpty || hasExceededMaxCommentLength) ? themeColors.icon : themeColors.textLight} />
</TouchableOpacity>
</Tooltip>
</View>
</View>
<View style={[styles.flexRow, styles.mt1]}>
<Button
small
style={[styles.mr2]}
nativeID={this.cancelButtonID}
onPress={this.deleteDraft}
text={this.props.translate('common.cancel')}
/>
<Button
small
success
isDisabled={hasExceededMaxCommentLength}
nativeID={this.saveButtonID}
style={[styles.mr2]}
onPress={this.publishDraft}
text={this.props.translate('common.saveChanges')}
/>
<ExceededCommentLength comment={this.state.draft} onExceededMaxCommentLength={this.setExceededMaxCommentLength} />
</View>

<ExceededCommentLength comment={this.state.draft} onExceededMaxCommentLength={this.setExceededMaxCommentLength} />
</View>
);
}
Expand Down
15 changes: 14 additions & 1 deletion src/styles/styles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1491,6 +1491,19 @@ const styles = {
flex: 1,
},

chatItemDraft: {
display: 'flex',
flexDirection: 'row',
Comment on lines +1495 to +1496
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
display: 'flex',
flexDirection: 'row',
...flex.dFlex,
...flex.flexRow,

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 only see a single ...flex.flerRow usage and lots of fledDirection: 'row' and it feels like anti patters. Can you link to the style guide for this? Spreading a single value doesn't give anything here, but it's even harder to type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hard-coding the same style in multiple places is an anti-pattern as it violates the DRY principle. We have a checklist item for that :

I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)

Copy link
Contributor Author

@terrysahaidak terrysahaidak May 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I feel that it's more about the usage of styles.flex1 instead of defining your own myStyle where you have only { flex: 1 }, but not about building a style object out of other objects completely.

paddingTop: 8,
paddingBottom: 8,
paddingLeft: 20,
paddingRight: 20,
},

chatItemReactionsDraftRight: {
marginLeft: 52,
},

// Be extremely careful when editing the compose styles, as it is easy to introduce regressions.
// Make sure you run the following tests against any changes: #12669
textInputCompose: addOutlineWidth({
Expand Down Expand Up @@ -1523,7 +1536,7 @@ const styles = {

editInputComposeSpacing: {
backgroundColor: themeColors.transparent,
marginVertical: 6,
marginVertical: 8,
},

// composer padding should not be modified unless thoroughly tested against the cases in this PR: #12669
Expand Down