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 10 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
24 changes: 6 additions & 18 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 React, {memo} from 'react';
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;
export default memo(UnreadActionIndicator);
12 changes: 7 additions & 5 deletions src/libs/actions/Report.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,13 +204,14 @@ function fetchChatReportsByIDs(chatList) {
*
* @param {Number} reportID
* @param {Number} sequenceNumber
* @param {Number} maxSequenceNumber
*/
function setLocalLastRead(reportID, sequenceNumber) {
function setLocalLastRead(reportID, sequenceNumber, maxSequenceNumber = 0) {
lastReadSequenceNumbers[reportID] = sequenceNumber;

// Update the report optimistically
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {
unreadActionCount: 0,
unreadActionCount: maxSequenceNumber > 0 ? maxSequenceNumber - sequenceNumber : 0,
lastVisitedTimestamp: Date.now(),
});
}
Expand Down Expand Up @@ -663,14 +664,15 @@ function addAction(reportID, text, file) {
*
* @param {Number} reportID
* @param {Number} sequenceNumber
* @param {Boolean} ignoreOrder
Copy link
Contributor

Choose a reason for hiding this comment

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

Just looking at the method docs, I'm not sure what ignoreOrder implies. Would you mind adding a more detailed description to the param?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup. 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.

*/
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);
setLocalLastRead(reportID, sequenceNumber, currentMaxSequenceNumber);

// Mark the report as not having any unread items
API.Report_UpdateLastRead({
Expand Down
6 changes: 5 additions & 1 deletion src/pages/home/report/ReportActionContextMenu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,9 @@ 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.isRequired,
};

const defaultProps = {
Expand All @@ -65,7 +68,7 @@ const defaultProps = {
};

const ReportActionContextMenu = (props) => {
const wrapperStyle = getReportActionContextMenuStyles(props.isMini);
const wrapperStyle = getReportActionContextMenuStyles(true);
return props.isVisible && (
<View style={wrapperStyle}>
{CONTEXT_ACTIONS.map(contextAction => (
Expand All @@ -74,6 +77,7 @@ const ReportActionContextMenu = (props) => {
text={contextAction.text}
isMini={props.isMini}
key={contextAction.text}
onPress={contextAction.text === 'Mark as Unread' ? props.onMarkAsUnread : () => {}}
/>
))}
</View>
Expand Down
12 changes: 10 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 @@ -30,10 +30,12 @@ const propTypes = {
icon: PropTypes.elementType.isRequired,
text: PropTypes.string.isRequired,
isMini: PropTypes.bool,
onPress: PropTypes.func,
};

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

const ReportActionContextMenuItem = (props) => {
Expand All @@ -42,7 +44,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 +57,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
14 changes: 13 additions & 1 deletion src/pages/home/report/ReportActionItem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ 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 ID of the report this action is on.
Expand All @@ -25,6 +26,12 @@ 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,

// If true, display the New indicator above this item
displayNewIndicator: PropTypes.bool.isRequired,

/* --- Onyx Props --- */
// List of betas for the current user.
betas: PropTypes.arrayOf(PropTypes.string),
Expand Down Expand Up @@ -56,7 +63,8 @@ class ReportActionItem extends Component {
shouldComponentUpdate(nextProps, nextState) {
return this.state.isPopoverVisible !== nextState.isPopoverVisible
|| this.props.displayAsGroup !== nextProps.displayAsGroup
|| !_.isEqual(this.props.action, nextProps.action);
|| !_.isEqual(this.props.action, nextProps.action)
|| this.props.displayNewIndicator !== nextProps.displayNewIndicator;
}

/**
Expand Down Expand Up @@ -107,6 +115,7 @@ class ReportActionItem extends Component {
<Hoverable>
{hovered => (
<View>
{this.props.displayNewIndicator && (<UnreadActionIndicator />)}
<View style={getReportActionItemStyles(hovered)}>
{!this.props.displayAsGroup
? <ReportActionItemSingle action={this.props.action} />
Expand All @@ -121,6 +130,7 @@ class ReportActionItem extends Component {
&& this.isInReportActionContextMenuBeta()
&& !this.state.isPopoverVisible
}
onMarkAsUnread={this.props.onMarkAsUnread}
isMini
/>
</View>
Expand All @@ -134,13 +144,15 @@ class ReportActionItem extends Component {
isVisible
reportID={-1}
reportActionID={-1}
onMarkAsUnread={() => {}}
/>
)}
>
<ReportActionContextMenu
isVisible={this.state.isPopoverVisible}
reportID={this.props.reportID}
reportActionID={this.props.action.sequenceNumber}
onMarkAsUnread={this.props.onMarkAsUnread}
/>
</PopoverWithMeasuredContent>
</View>
Expand Down
44 changes: 24 additions & 20 deletions src/pages/home/report/ReportActionsView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import React from 'react';
import {
Animated,
View,
Keyboard,
AppState,
Expand All @@ -11,7 +10,6 @@ import _ from 'underscore';
import lodashGet from 'lodash.get';
import {withOnyx} from 'react-native-onyx';
import Text from '../../../components/Text';
import UnreadActionIndicator from '../../../components/UnreadActionIndicator';
import {
fetchActions,
updateLastReadActionID,
Expand Down Expand Up @@ -39,6 +37,9 @@ const propTypes = {
report: PropTypes.shape({
// Number of actions unread
unreadActionCount: PropTypes.number,

// Number of the last sequence
maxSequenceNumber: PropTypes.number,
}),

// Array of report actions for this report
Expand All @@ -54,6 +55,7 @@ const propTypes = {
const defaultProps = {
report: {
unreadActionCount: 0,
maxSequenceNumber: 0,
},
reportActions: {},
session: {},
Expand All @@ -68,19 +70,18 @@ class ReportActionsView extends React.Component {
this.scrollToListBottom = this.scrollToListBottom.bind(this);
this.recordMaxAction = this.recordMaxAction.bind(this);
this.onVisibilityChange = this.onVisibilityChange.bind(this);
this.onMarkAsUnread = this.onMarkAsUnread.bind(this);
this.sortedReportActions = this.updateSortedReportActions();
this.loadMoreChats = this.loadMoreChats.bind(this);
this.sortedReportActions = [];
this.timers = [];
this.unreadIndicatorOpacity = new Animated.Value(1);

// Helper variable that keeps track of the unread action count before it updates to zero
this.unreadActionCount = 0;

// Helper variable that prevents the unread indicator to show up for new messages
// received while the report is still active
this.shouldShowUnreadActionIndicator = true;

this.state = {
unreadActionCount: 0,
isLoadingMoreChats: false,
};
}
Expand All @@ -102,6 +103,10 @@ class ReportActionsView extends React.Component {
return true;
}

if (nextState.unreadActionCount !== this.state.unreadActionCount) {
return true;
}

if (nextState.isLoadingMoreChats !== this.state.isLoadingMoreChats) {
return true;
}
Expand All @@ -113,6 +118,7 @@ class ReportActionsView extends React.Component {
// We have switched to a new report
if (prevProps.reportID !== this.props.reportID) {
this.reset(prevProps.reportID);
this.shouldShowUnreadActionIndicator = true;
return;
}

Expand Down Expand Up @@ -155,6 +161,13 @@ class ReportActionsView extends React.Component {
}
}

onMarkAsUnread(actionIndex) {
updateLastReadActionID(this.props.reportID, actionIndex, true);
this.setState({
unreadActionCount: this.props.report.maxSequenceNumber - actionIndex,
});
}

/**
* Checks if the unreadActionIndicator should be shown.
* If it does, starts a timeout for the fading out animation and creates
Expand All @@ -165,17 +178,9 @@ class ReportActionsView extends React.Component {
return;
}

this.unreadActionCount = this.props.report.unreadActionCount;

if (this.unreadActionCount > 0) {
this.unreadIndicatorOpacity = new Animated.Value(1);
this.timers.push(setTimeout(() => {
Animated.timing(this.unreadIndicatorOpacity, {
toValue: 0,
useNativeDriver: false,
}).start();
}, 3000));
}
this.setState({
unreadActionCount: this.props.report.unreadActionCount,
});

this.shouldShowUnreadActionIndicator = false;
}
Expand Down Expand Up @@ -335,15 +340,14 @@ class ReportActionsView extends React.Component {
// <InvertedFlatList /> are implemented on native and web/desktop which leads to
// the unread indicator on native to render below the message instead of above it.
<View>
{this.unreadActionCount > 0 && index === this.unreadActionCount - 1 && (
<UnreadActionIndicator animatedOpacity={this.unreadIndicatorOpacity} />
)}
<ReportActionItem
reportID={this.props.reportID}
action={item.action}
displayAsGroup={this.isConsecutiveActionMadeByPreviousActor(index)}
onLayout={onLayout}
needsLayoutCalculation={needsLayoutCalculation}
onMarkAsUnread={() => this.onMarkAsUnread(item.action.sequenceNumber - 1)}
displayNewIndicator={this.state.unreadActionCount > 0 && index === this.state.unreadActionCount - 1}
/>
</View>
);
Expand Down
7 changes: 4 additions & 3 deletions src/styles/styles.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -768,7 +768,6 @@ const styles = {
left: 0,
top: 0,
bottom: 0,
zIndex: 2,
},

sidebarVisible: {
Expand Down Expand Up @@ -947,6 +946,7 @@ const styles = {
...positioning.tn4,
...positioning.r4,
position: 'absolute',
zIndex: 999,
},

reportActionContextMenuText: {
Expand Down Expand Up @@ -1034,13 +1034,14 @@ const styles = {

unreadIndicatorContainer: {
position: 'absolute',
top: -10,
top: -5,
left: 0,
width: '100%',
height: 20,
height: 10,
paddingHorizontal: 20,
flexDirection: 'row',
alignItems: 'center',
zIndex: 1,
},

unreadIndicatorLine: {
Expand Down
Loading