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

[HOLD] [WIP] Introduce "Mark as unread" functionality in Expensify.cash #1774

Closed
wants to merge 15 commits into from
Closed

[HOLD] [WIP] Introduce "Mark as unread" functionality in Expensify.cash #1774

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
1cc91b4
Handle Mark as Unread action
Gonals Mar 4, 2021
5ef40a4
make the call correctly
Gonals Mar 5, 2021
a02bcd2
Merge master
Gonals Mar 5, 2021
1a5705b
remove animation and refactor new marker
Gonals Mar 11, 2021
74daa6d
merge master
Gonals Mar 11, 2021
dad6a80
merge master
Gonals Mar 11, 2021
36eae89
merge master
Gonals Mar 15, 2021
4076540
cleanup
Gonals Mar 16, 2021
814f85e
cleanup
Gonals Mar 16, 2021
a17f189
Make sure we don't move the New marker when new messages come in
Gonals Mar 16, 2021
60aa19b
Big revamp to remove some local variables and clean up code
Gonals Mar 17, 2021
9506213
remove memo and unused report
Gonals Mar 18, 2021
939c1ea
Merge branch 'master' into alberto-markUnread
Gonals Mar 18, 2021
6cebb71
linter
Gonals Mar 18, 2021
0c43eb9
remove log file
Gonals Mar 18, 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
20 changes: 4 additions & 16 deletions src/components/UnreadActionIndicator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,29 +1,17 @@
import React from 'react';
import {Animated, View} from 'react-native';
import PropTypes from 'prop-types';
import {View} from 'react-native';
import styles from '../styles/styles';
import Text from './Text';

const propTypes = {
// Animated opacity
// eslint-disable-next-line react/forbid-prop-types
animatedOpacity: PropTypes.object.isRequired,
};

const UnreadActionIndicator = props => (
<Animated.View style={[
styles.unreadIndicatorContainer,
{opacity: props.animatedOpacity},
]}
>
const UnreadActionIndicator = () => (
<View style={styles.unreadIndicatorContainer}>
<View style={styles.unreadIndicatorLine} />
<Text style={styles.unreadIndicatorText}>
NEW
</Text>
</Animated.View>
</View>
);

UnreadActionIndicator.propTypes = propTypes;
UnreadActionIndicator.displayName = 'UnreadActionIndicator';

export default UnreadActionIndicator;
10 changes: 6 additions & 4 deletions src/libs/actions/Report.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ function setLocalLastRead(reportID, sequenceNumber) {

// Update the report optimistically
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {
unreadActionCount: 0,
unreadActionCount: Math.max(reportMaxSequenceNumbers[reportID] - sequenceNumber, 0),
lastVisitedTimestamp: Date.now(),
});
}
Expand Down Expand Up @@ -669,16 +669,18 @@ function addAction(reportID, text, file) {
*
* @param {Number} reportID
* @param {Number} sequenceNumber
* @param {Boolean} ignoreOrder If set to true, we will not enforce the latest read action to be at the bottom of the
* chat.
*/
function updateLastReadActionID(reportID, sequenceNumber) {
function updateLastReadActionID(reportID, sequenceNumber, ignoreOrder = false) {
const currentMaxSequenceNumber = reportMaxSequenceNumbers[reportID];
if (sequenceNumber < currentMaxSequenceNumber) {
if (!ignoreOrder && sequenceNumber < currentMaxSequenceNumber) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than add an ignoreOrder flag that basically says "don't run this function" would it make more sense to not call the function in the first place.

Copy link
Contributor Author

@Gonals Gonals Mar 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

? It only skips checking the order, though. It runs the rest of the function. Basically, it prevents the function from checking that the new "latest read" is at the end of the conversation. That check is relevant when new messages come in, but not when we are manually marking messages as unread.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm maybe we should remove that logic as well.. this logic is confusing and even with the added comment I'm having some trouble understanding in what other contexts this should be used.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I'm trying to think of a better suggestion for this...

Copy link
Contributor

@marcaaron marcaaron Mar 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Ok so here is my confusion and would like to get @tgolen's thoughts to see if I'm missing something.

I'm not sure I understand why we need to get the last action from the sorted actions here to mark everything as read...

https://github.com/Expensify/Expensify.cash/blob/c5cb5076dc092ee5246e3410e4557fc2d55b4a88/src/pages/home/report/ReportActionsView.js#L266-L278

Here we are looking at the sequenceNumber on the last action... but this information is already available to the actions/Report and maxVisibleSequenceNumber is kind of a misnomer since it has nothing to do with visibility.

We are filtering out the "loading" actions, but that too could be done in the actions/Report file I think.

If that's true then we wouldn't need an ignoreOrder and could make sequenceNumber optional and the logic for this method could then end up being

function updateLastReadActionID(reportID, sequenceNumber) {
    const currentMaxSequenceNumber = reportMaxSequenceNumbers[reportID];
    const lastReadSequenceNumber = sequenceNumber 
        ? sequenceNumber 
        : currentMaxSequenceNumber;
    //... set the local last read 
}

Not sure if we need to do this change right now, but it seems more intuitive to "set to the most recent sequenceNumber when none is passed or use the one specified".

Copy link
Contributor

Choose a reason for hiding this comment

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

I want to dig into this a little so I'm going to pull the branch down and dive into it. I want to make sure I understand everything before offering up further comments on this thread. Thanks for being patient with me!

Copy link
Contributor

Choose a reason for hiding this comment

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

it seems more intuitive to "set to the most recent sequenceNumber when none is passed or use the one specified"

I love coming at it from this angle, and I agree it is much more intuitive.

but that too could be done in the actions/Report file I think.

I briefly looked at this, and I am pretty concerned that changing reportMaxSequenceNumbers to not include loading actions has a high risk of breaking things. I would be more comfortable with keeping a separate reportMaxSequenceNumbersForNonLoadingActions (lol at that ugly name).

It makes a lot of sense to me to move this logic from the view into the action file: https://github.com/Expensify/Expensify.cash/blob/c5cb5076dc092ee5246e3410e4557fc2d55b4a88/src/pages/home/report/ReportActionsView.js#L266-L278

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'm afraid I'm the one who is lost now.
What do you mean by loading/not loading actions?

Copy link
Contributor

Choose a reason for hiding this comment

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

In the ReportActionsView.js, when it calls updateLastReadActionID, it is passing the maxVisibileSequenceNumber. It gets that number by getting all the report actions and filtering out the ones with action.loading. This is what we are referring to.

What I would like to see for a solution is to have ReportActionsView.js updated so that all it does is this:

    recordMaxAction() {
        updateLastReadActionID(this.props.reportID);
    }

and also:

onMarkAsUnread={() => updateLastReadActionID(this.props.reportID, item.action.sequenceNumber - 1)}

All the rest of the logic will then be encapsulated into the actions/Report.js file.

Does that help clarify what we are asking for? It doesn't spell out the changes needed in Report.js, so let me know if you need help with that as well. I think this will be enough to point you in the right direction though.

return;
}

setLocalLastRead(reportID, sequenceNumber);

// Mark the report as not having any unread items
// Mark the unread items in the report
API.Report_UpdateLastRead({
accountID: currentUserAccountID,
reportID,
Expand Down
79 changes: 44 additions & 35 deletions src/pages/home/report/ReportActionContextMenu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,41 +7,6 @@ import {
import getReportActionContextMenuStyles from '../../../styles/getReportActionContextMenuStyles';
import ReportActionContextMenuItem from './ReportActionContextMenuItem';

/**
* A list of all the context actions in this menu.
*/
const CONTEXT_ACTIONS = [
// Copy to clipboard
{
text: 'Copy to Clipboard',
icon: Clipboard,
},

// Copy chat link
{
text: 'Copy Link',
icon: LinkCopy,
},

// Mark as Unread
{
text: 'Mark as Unread',
icon: Mail,
},

// Edit Comment
{
text: 'Edit Comment',
icon: Pencil,
},

// Delete Comment
{
text: 'Delete Comment',
icon: Trashcan,
},
];

const propTypes = {
// The ID of the report this report action is attached to.
// eslint-disable-next-line react/no-unused-prop-types
Expand All @@ -57,14 +22,57 @@ const propTypes = {

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

// Function to trigger when we try to mark a message as unread
onMarkAsUnread: PropTypes.func,
};

const defaultProps = {
isMini: false,
isVisible: false,
onMarkAsUnread: () => {},
};

const ReportActionContextMenu = (props) => {
/**
* A list of all the context actions in this menu.
*/
const CONTEXT_ACTIONS = [
Copy link
Contributor

Choose a reason for hiding this comment

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

NAB, one small side effect of this change is that we are now recreating this object with each render. I don't think it will cause problems tho.

Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than recreating this object with every render, I think we should convert this to a class component, make contextActions a property of the class, then set it once in componentDidMount

Copy link
Contributor

Choose a reason for hiding this comment

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

Or the constructor()

// Copy to clipboard
{
text: 'Copy to Clipboard',
icon: Clipboard,
onPress: () => {},
},

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

// Mark as Unread
{
text: 'Mark as Unread',
icon: Mail,
onPress: props.onMarkAsUnread,
},

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

// Delete Comment
{
text: 'Delete Comment',
icon: Trashcan,
onPress: () => {},
},
];
const wrapperStyle = getReportActionContextMenuStyles(props.isMini);
return props.isVisible && (
<View style={wrapperStyle}>
Expand All @@ -74,6 +82,7 @@ const ReportActionContextMenu = (props) => {
text={contextAction.text}
isMini={props.isMini}
key={contextAction.text}
onPress={contextAction.onPress}
/>
))}
</View>
Expand Down
20 changes: 18 additions & 2 deletions src/pages/home/report/ReportActionContextMenuItem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,13 +27,23 @@ function getButtonState(isHovered = false, isPressed = false) {
}

const propTypes = {

// Icon to display in the menu
icon: PropTypes.elementType.isRequired,

// Text for the action
text: PropTypes.string.isRequired,

// If true, we are displaying the mini hover-menu
isMini: PropTypes.bool,

// Function to trigger when the action is pressed
onPress: PropTypes.func,
};

const defaultProps = {
isMini: false,
onPress: () => {},
};

const ReportActionContextMenuItem = (props) => {
Expand All @@ -42,7 +52,10 @@ const ReportActionContextMenuItem = (props) => {
props.isMini
? (
<Tooltip text={props.text}>
<Pressable style={({hovered, pressed}) => getButtonStyle(getButtonState(hovered, pressed))}>
<Pressable
style={({hovered, pressed}) => getButtonStyle(getButtonState(hovered, pressed))}
onPress={props.onPress}
>
{({hovered, pressed}) => (
<Icon
src={props.icon}
Expand All @@ -52,7 +65,10 @@ const ReportActionContextMenuItem = (props) => {
</Pressable>
</Tooltip>
) : (
<Pressable style={({hovered, pressed}) => getButtonStyle(getButtonState(hovered, pressed))}>
<Pressable
style={({hovered, pressed}) => getButtonStyle(getButtonState(hovered, pressed))}
onPress={props.onPress}
>
{({hovered, pressed}) => (
<>
<Icon
Expand Down
29 changes: 29 additions & 0 deletions src/pages/home/report/ReportActionItem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,12 @@ import PopoverWithMeasuredContent from '../../../components/PopoverWithMeasuredC
import ReportActionItemSingle from './ReportActionItemSingle';
import ReportActionItemGrouped from './ReportActionItemGrouped';
import ReportActionContextMenu from './ReportActionContextMenu';
import UnreadActionIndicator from '../../../components/UnreadActionIndicator';

const propTypes = {
// The index of this action in the flatList
index: PropTypes.number.isRequired,

// The ID of the report this action is on.
reportID: PropTypes.number.isRequired,

Expand All @@ -25,13 +29,25 @@ const propTypes = {
// Should the comment have the appearance of being grouped with the previous comment?
displayAsGroup: PropTypes.bool.isRequired,

// Function to trigger when mark as unread is selected
onMarkAsUnread: PropTypes.func.isRequired,

/* --- Onyx Props --- */
// List of betas for the current user.
betas: PropTypes.arrayOf(PropTypes.string),

// The report currently being looked at
report: PropTypes.shape({
// Number of actions unread
unreadActionCount: PropTypes.number,
}),
};

const defaultProps = {
betas: {},
report: {
unreadActionCount: 0,
},
};

class ReportActionItem extends Component {
Expand All @@ -54,8 +70,13 @@ class ReportActionItem extends Component {
}

shouldComponentUpdate(nextProps, nextState) {
const hasNewDisplayChanged = nextProps.report.unreadActionCount > 0
&& ((this.props.index === this.props.report.unreadActionCount - 1)
!== (nextProps.index === nextProps.report.unreadActionCount - 1));

return this.state.isPopoverVisible !== nextState.isPopoverVisible
|| this.props.displayAsGroup !== nextProps.displayAsGroup
|| hasNewDisplayChanged
|| !_.isEqual(this.props.action, nextProps.action);
}

Expand Down Expand Up @@ -102,11 +123,14 @@ class ReportActionItem extends Component {
}

render() {
const displayNewIndicator = this.props.report.unreadActionCount > 0
&& this.props.index === this.props.report.unreadActionCount - 1;
return (
<PressableWithSecondaryInteraction onSecondaryInteraction={this.showPopover}>
<Hoverable>
{hovered => (
<View>
{displayNewIndicator && <UnreadActionIndicator />}
<View style={getReportActionItemStyles(hovered)}>
{!this.props.displayAsGroup
? <ReportActionItemSingle action={this.props.action} />
Expand All @@ -121,6 +145,7 @@ class ReportActionItem extends Component {
&& this.isInReportActionContextMenuBeta()
&& !this.state.isPopoverVisible
}
onMarkAsUnread={this.props.onMarkAsUnread}
isMini
/>
</View>
Expand All @@ -141,6 +166,7 @@ class ReportActionItem extends Component {
isVisible={this.state.isPopoverVisible}
reportID={this.props.reportID}
reportActionID={this.props.action.sequenceNumber}
onMarkAsUnread={this.props.onMarkAsUnread}
/>
</PopoverWithMeasuredContent>
</View>
Expand All @@ -158,4 +184,7 @@ export default withOnyx({
betas: {
key: ONYXKEYS.BETAS,
},
report: {
key: ({reportID}) => `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
},
})(ReportActionItem);
Loading