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

Edit Comments #2320

Merged
merged 35 commits into from
May 11, 2021
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
b318a69
Edit Comments - fundamental API portions, version allows to edit to a…
yuwenmemon Mar 22, 2021
41e885d
Get dirty editing UI in place
yuwenmemon Mar 23, 2021
8f1fb48
Fix conflicts?
yuwenmemon Apr 9, 2021
94053ea
reall fix conflicts
yuwenmemon Apr 9, 2021
bf1e59c
Really non-performant comment editing
yuwenmemon Apr 9, 2021
8500513
Make sure we have reportID in grouped report action item
yuwenmemon Apr 13, 2021
8f8fd1c
Fix the stylings for a message being edited
yuwenmemon Apr 14, 2021
00fae5d
Make it so that clicking the edit comment button again cancels the edit
yuwenmemon Apr 14, 2021
e967c38
Add keypress handlers
yuwenmemon Apr 14, 2021
f94a9c4
Fix conflicts
yuwenmemon Apr 14, 2021
cc295ae
Revert some unneccessary measureContent crap I edited out
yuwenmemon Apr 14, 2021
235cf83
Fix a couple bugs
yuwenmemon Apr 14, 2021
40c7c30
Add edited indicator
yuwenmemon Apr 15, 2021
ca5d13e
Add some comments, get this in more a polished state, PR comments
yuwenmemon Apr 23, 2021
32103ae
Fix conflicts
yuwenmemon Apr 23, 2021
5d0536f
hidePopover
yuwenmemon Apr 28, 2021
2594d4f
Fix conflicts
yuwenmemon Apr 28, 2021
ce406cd
Playing around with ReportScrollManager
yuwenmemon Apr 28, 2021
a29f272
Scroll touch screens to comment when editing a comment
yuwenmemon Apr 30, 2021
da33bc6
Show/hide compose box when focusing on editing comment in mobile
yuwenmemon Apr 30, 2021
1ef1bce
Linting
yuwenmemon Apr 30, 2021
ffe69f1
PR Comments
yuwenmemon May 4, 2021
1aca583
Remove uneccessary blur event
yuwenmemon May 4, 2021
e1185d2
Fix Conflicts
yuwenmemon May 5, 2021
4fd791d
Update when recieving an edited message via pusher event
yuwenmemon May 6, 2021
243819e
Hide report compose when editing comment on all small screens
yuwenmemon May 6, 2021
4809f11
Line length
yuwenmemon May 6, 2021
12ce6e8
Fix conflicts
yuwenmemon May 10, 2021
a2259c2
Add doc for scrollToIndex
yuwenmemon May 10, 2021
932565a
Fix conflictrs
yuwenmemon May 10, 2021
dfae939
Dumb auto styling fix
yuwenmemon May 10, 2021
e7abc09
PR Comments
yuwenmemon May 10, 2021
f4156d0
Make sure we have a reportActionID if we're going to allow you to edi…
yuwenmemon May 10, 2021
89f7537
Resolve console error when textInput has no focus method
yuwenmemon May 10, 2021
8011820
Keyboard persists taps handled
yuwenmemon May 11, 2021
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
1 change: 1 addition & 0 deletions src/ONYXKEYS.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ export default {
REPORT: 'report_',
REPORT_ACTIONS: 'reportActions_',
REPORT_DRAFT_COMMENT: 'reportDraftComment_',
REPORT_ACTIONS_DRAFTS: 'reportActionsDrafts_',
REPORT_USER_IS_TYPING: 'reportUserIsTyping_',
REPORT_IOUS: 'reportIOUs_',
},
Expand Down
4 changes: 2 additions & 2 deletions src/components/PopoverWithMeasuredContent.js
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ class PopoverWithMeasuredContent extends Component {
{...this.props}
anchorPosition={this.calculateAdjustedAnchorPosition()}
>
{this.props.children}
{this.props.measureContent()}
roryabraham marked this conversation as resolved.
Show resolved Hide resolved
</Popover>
) : (

Expand All @@ -137,7 +137,7 @@ class PopoverWithMeasuredContent extends Component {
but we can't measure its dimensions without first rendering it.
*/
<View style={styles.invisible} onLayout={this.measurePopover}>
{this.props.measureContent()}
{this.props.children}
</View>
);
}
Expand Down
14 changes: 14 additions & 0 deletions src/libs/API.js
Original file line number Diff line number Diff line change
Expand Up @@ -543,6 +543,19 @@ function Report_TogglePinned(parameters) {
return Network.post(commandName, parameters);
}

/**
* @param {Object} parameters
* @param {Number} parameters.reportID
* @param {String} parameters.reportActionID
* @param {String} parameters.reportComment
* @return {Promise}
*/
function Report_EditComment(parameters) {
const commandName = 'Report_EditComment';
requireParameters(['reportID', 'reportActionID', 'reportComment'], parameters, commandName);
return Network.post(commandName, parameters);
}

/**
* @param {Object} parameters
* @param {Number} parameters.accountID
Expand Down Expand Up @@ -676,6 +689,7 @@ export {
Report_AddComment,
Report_GetHistory,
Report_TogglePinned,
Report_EditComment,
Report_UpdateLastRead,
ResendValidateCode,
ResetPassword,
Expand Down
25 changes: 25 additions & 0 deletions src/libs/actions/Report.js
Original file line number Diff line number Diff line change
Expand Up @@ -896,6 +896,29 @@ NetworkConnection.onReconnect(() => {
fetchAll(false);
});

function editReportComment(reportID, reportAction, htmlForNewComment) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing a method doc here

// Optimistically update the report action with the new message
const sequenceNumber = reportAction.sequenceNumber;
Copy link
Contributor

Choose a reason for hiding this comment

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

I was just reviewing @sketchydroide's delete comment PR, and I thought the way he approached this syntactically was a bit cleaner (and has better error handling):

function editReportComment(reportID, reportAction, htmlForNewComment) {
    // Optimistically update Onyx
    actionToMerge = {}'
    const oldMessage = {...reportAction.message};
    actionToMerge[reportAction.sequenceNumber] = {
        ...reportAction,
        message: [
            {
                type: 'COMMENT',
                html: htmlForNewComment,
                text: htmlForNewComment.replace(/<[^>]*>?/gm, ''),
            },
        ],
    };
    
    Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`, actionToMerge);
    
    // Make the API request
    API.Report_EditComment({
        reportID,
        reportActionID: reportAction.reportActionID,
        reportComment: htmlForNewComment,
    })
        .catch(() => {
            // If it fails, reset Onyx
            actionToMerge[reportAction.sequenceNumber] = {
                ...reportAction,
                message: oldMessage,
            };

            Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`, reportActionsToMerge);
        });
}

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 like the error handling, but I actually find my way to be less confusing. Maybe that's just because I wrote it so I'm familiar with it, but it's a little more clear what's exactly is being edited.

const newReportAction = {...reportAction};
const actionToMerge = {};
newReportAction.message[0].isEdited = true;
newReportAction.message[0].html = htmlForNewComment;
newReportAction.message[0].text = htmlForNewComment.replace(/<[^>]*>?/gm, '');
roryabraham marked this conversation as resolved.
Show resolved Hide resolved
actionToMerge[sequenceNumber] = newReportAction;
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS}${reportID}`, actionToMerge);

// Persist the updated report comment
API.Report_EditComment({
reportID,
reportActionID: reportAction.reportActionID,
reportComment: htmlForNewComment,
});
}

function saveReportActionDraft(reportID, reportActionID, draftMessage) {
Onyx.set(`${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}${reportID}_${reportActionID}`, draftMessage);
}

export {
fetchAll,
fetchActions,
Expand All @@ -909,4 +932,6 @@ export {
broadcastUserIsTyping,
togglePinnedState,
updateCurrentlyViewedReportID,
editReportComment,
saveReportActionDraft,
};
199 changes: 117 additions & 82 deletions src/pages/home/report/ReportActionContextMenu.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,77 +3,17 @@ import React from 'react';
import {View} from 'react-native';
import PropTypes from 'prop-types';
import lodashGet from 'lodash/get';
import {withOnyx} from 'react-native-onyx';
import {
Clipboard as ClipboardIcon, LinkCopy, Mail, Pencil, Trashcan, Checkmark,
} from '../../../components/Icon/Expensicons';
import getReportActionContextMenuStyles from '../../../styles/getReportActionContextMenuStyles';
import ReportActionContextMenuItem from './ReportActionContextMenuItem';
import {saveReportActionDraft} from '../../../libs/actions/Report';
import ReportActionPropTypes from './ReportActionPropTypes';
import Clipboard from '../../../libs/Clipboard';
import {isReportMessageAttachment} from '../../../libs/reportUtils';

/**
* A list of all the context actions in this menu.
*/
const CONTEXT_ACTIONS = [
// Copy to clipboard
{
text: 'Copy to Clipboard',
icon: ClipboardIcon,
successText: 'Copied!',
successIcon: Checkmark,
shouldShow: true,

// If return value is true, we switch the `text` and `icon` on
// `ReportActionContextMenuItem` with `successText` and `successIcon` which will fallback to
// the `text` and `icon`
onPress: (action) => {
const message = _.last(lodashGet(action, 'message', null));
const html = lodashGet(message, 'html', '');
const text = lodashGet(message, 'text', '');
const isAttachment = _.has(action, 'isAttachment')
? action.isAttachment
: isReportMessageAttachment(text);
if (!isAttachment) {
Clipboard.setString(text);
} else {
Clipboard.setString(html);
}
},
},

// Copy chat link
{
text: 'Copy Link',
icon: LinkCopy,
shouldShow: false,
onPress: () => {},
},

// Mark as Unread
{
text: 'Mark as Unread',
icon: Mail,
shouldShow: false,
onPress: () => {},
},

// Edit Comment
{
text: 'Edit Comment',
icon: Pencil,
shouldShow: false,
onPress: () => {},
},

// Delete Comment
{
text: 'Delete Comment',
icon: Trashcan,
shouldShow: false,
onPress: () => {},
},
];
import ONYXKEYS from '../../../ONYXKEYS';

const propTypes = {
// The ID of the report this report action is attached to.
Expand All @@ -89,34 +29,129 @@ const propTypes = {

// Controls the visibility of this component.
isVisible: PropTypes.bool,

/* Onyx Props */
// The session of the logged in person
session: PropTypes.shape({
// Email of the logged in person
email: PropTypes.string,
}),

// Draft message - if this is set the comment is in 'edit' mode
draftMessage: PropTypes.string,
};

const defaultProps = {
reportAction: {},
isMini: false,
isVisible: false,
session: {},
};

const ReportActionContextMenu = (props) => {
const wrapperStyle = getReportActionContextMenuStyles(props.isMini);
return props.isVisible && (
<View style={wrapperStyle}>
{CONTEXT_ACTIONS.map(contextAction => contextAction.shouldShow && (
<ReportActionContextMenuItem
icon={contextAction.icon}
text={contextAction.text}
successIcon={contextAction.successIcon}
successText={contextAction.successText}
isMini={props.isMini}
onPress={() => contextAction.onPress(props.reportAction)}
key={contextAction.text}
/>
))}
</View>
);
};
class ReportActionContextMenu extends React.Component {
/**
* A list of all the context actions in this menu.
*/
CONTEXT_ACTIONS = [
roryabraham marked this conversation as resolved.
Show resolved Hide resolved
// Copy to clipboard
{
text: 'Copy to Clipboard',
icon: ClipboardIcon,
successText: 'Copied!',
successIcon: Checkmark,
shouldShow: true,

// If return value is true, we switch the `text` and `icon` on
// `ReportActionContextMenuItem` with `successText` and `successIcon` which will fallback to
// the `text` and `icon`
onPress: (action) => {
const message = _.last(lodashGet(action, 'message', null));
const html = lodashGet(message, 'html', '');
const text = lodashGet(message, 'text', '');
const isAttachment = _.has(action, 'isAttachment')
? action.isAttachment
: isReportMessageAttachment(text);
if (!isAttachment) {
Clipboard.setString(text);
} else {
Clipboard.setString(html);
}
},
},

// Copy chat link
{
text: 'Copy Link',
icon: LinkCopy,
shouldShow: false,
onPress: () => {},
},

// Mark as Unread
{
text: 'Mark as Unread',
icon: Mail,
shouldShow: false,
onPress: () => {},
},

// Edit Comment
{
text: 'Edit Comment',
icon: Pencil,
shouldShow: this.props.reportAction.actorEmail === this.props.session.email
&& !isReportMessageAttachment(this.getActionText()),
onPress: () => {
saveReportActionDraft(
this.props.reportID,
this.props.reportAction.reportActionID,
_.isEmpty(this.props.draftMessage) ? this.getActionText() : '',
);
},
},

// Delete Comment
{
text: 'Delete Comment',
icon: Trashcan,
shouldShow: false,
onPress: () => {},
},
];

getActionText() {
roryabraham marked this conversation as resolved.
Show resolved Hide resolved
const message = _.last(lodashGet(this.props.reportAction, 'message', null));
return lodashGet(message, 'text', '');
}

render() {
const wrapperStyle = getReportActionContextMenuStyles(this.props.isMini);
return this.props.isVisible && (
<View style={wrapperStyle}>
{this.CONTEXT_ACTIONS.map(contextAction => contextAction.shouldShow && (
<ReportActionContextMenuItem
icon={contextAction.icon}
text={contextAction.text}
successIcon={contextAction.successIcon}
successText={contextAction.successText}
isMini={this.props.isMini}
onPress={() => contextAction.onPress(this.props.reportAction)}
key={contextAction.text}
/>
))}
</View>
);
}
}

ReportActionContextMenu.propTypes = propTypes;
ReportActionContextMenu.defaultProps = defaultProps;

export default ReportActionContextMenu;
export default withOnyx({
session: {
key: ONYXKEYS.SESSION,
},
draftMessage: {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you want to bind this component to the draftMessage in Onyx. Since the parent ReportActionItem is already bound to the draftMessage in Onyx, we can just pass that prop into here directly from the parent.

key: ({reportID, reportAction}) => `${ONYXKEYS.COLLECTION.REPORT_ACTIONS_DRAFTS}${reportID}_${reportAction.reportActionID}`,
},
})(ReportActionContextMenu);
1 change: 0 additions & 1 deletion src/pages/home/report/ReportActionContextMenuItem.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,6 @@ class ReportActionContextMenuItem extends Component {
}
}


ReportActionContextMenuItem.propTypes = propTypes;
ReportActionContextMenuItem.defaultProps = defaultProps;
ReportActionContextMenuItem.displayName = 'ReportActionContextMenuItem';
Expand Down
Loading