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 all 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 : {}}>
<ReportActionItemReactions
reactions={reactions}
toggleReaction={this.toggleReaction}
/>
</View>
)}
</>
);
}

/**
* Get ReportActionItem with a proper wrapper
luacmartins marked this conversation as resolved.
Show resolved Hide resolved
* @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;
175 changes: 103 additions & 72 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,79 +262,103 @@ class ReportActionItemMessageEdit extends React.Component {
render() {
const hasExceededMaxCommentLength = this.state.hasExceededMaxCommentLength;
return (
<View style={styles.chatItemMessage}>
<View
style={[
styles.chatItemComposeBox,
styles.flexRow,
this.state.isFocused ? styles.chatItemComposeBoxFocusedColor : styles.chatItemComposeBoxColor,
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.editChatItemEmojiWrapper}>
<EmojiPickerButton
isDisabled={this.props.shouldDisableEmojiPicker}
onModalHide={() => InteractionManager.runAfterInteractions(() => this.textInput.focus())}
onEmojiSelected={this.addEmojiToTextBox}
nativeID={this.emojiButtonID}
/>
<>
<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,
}}
>
{({hovered, pressed}) => (
<Icon src={Expensicons.Close} fill={StyleUtils.getIconFillColor(getButtonState(hovered, pressed))} />
)}
</Pressable>
</Tooltip>
</View>
<View
style={[
this.state.isFocused ? styles.chatItemComposeBoxFocusedColor : styles.chatItemComposeBoxColor,
styles.flexRow,
styles.flex1,
styles.chatItemComposeBox,
hasExceededMaxCommentLength && styles.borderColorDanger,
]}
>
<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}
onModalHide={() => InteractionManager.runAfterInteractions(() => this.textInput.focus())}
onEmojiSelected={this.addEmojiToTextBox}
nativeID={this.emojiButtonID}
/>
</View>

<View style={styles.alignSelfEnd}>
<Tooltip text={this.props.translate('common.saveChanges')}>
<TouchableOpacity
style={[styles.chatItemSubmitButton,
hasExceededMaxCommentLength ? {} : styles.buttonSuccess,
]}

onPress={this.publishDraft}
hitSlop={{
top: 3, right: 3, bottom: 3, left: 3,
}}
nativeID={this.saveButtonID}
disabled={hasExceededMaxCommentLength}
>
<Icon src={Expensicons.Checkmark} fill={hasExceededMaxCommentLength ? themeColors.icon : themeColors.textLight} />
</TouchableOpacity>
</Tooltip>
</View>
</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>
</View>
<ExceededCommentLength comment={this.state.draft} onExceededMaxCommentLength={this.setExceededMaxCommentLength} />
</>
);
}
}
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