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

Simplify new line indicator and message badge #10582

Merged
merged 7 commits into from
Aug 31, 2022
Merged

Simplify new line indicator and message badge #10582

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
0107ad3
Fix the new line marker and floating message indicator
marcaaron Aug 25, 2022
c78fd56
Fix issues
marcaaron Aug 25, 2022
ebd4ac2
remove logs
marcaaron Aug 25, 2022
cc2a820
remove outdated comment
marcaaron Aug 25, 2022
7451e17
Improve documentation
marcaaron Aug 25, 2022
64eed1c
improve comments in componentDidUpdate
marcaaron Aug 29, 2022
8b72671
Update comment
marcaaron Aug 29, 2022
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
2 changes: 1 addition & 1 deletion src/languages/en.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ export default {
beginningOfChatHistoryPolicyExpenseChatPartTwo: ' and ',
beginningOfChatHistoryPolicyExpenseChatPartThree: ' starts here! 🎉 This is the place to chat, request money and settle up.',
},
newMessageCount: ({count}) => `${count} new message${count > 1 ? 's' : ''}`,
newMessages: 'New messages',
reportTypingIndicator: {
isTyping: 'is typing...',
areTyping: 'are typing...',
Expand Down
2 changes: 1 addition & 1 deletion src/languages/es.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ export default {
beginningOfChatHistoryPolicyExpenseChatPartTwo: ' y ',
beginningOfChatHistoryPolicyExpenseChatPartThree: ' empieza aquí! :tada: Este es el lugar donde chatear, pedir dinero y pagar.',
},
newMessageCount: ({count}) => `${count} mensaje${count > 1 ? 's' : ''} nuevo${count > 1 ? 's' : ''}`,
newMessages: 'Mensajes nuevos',
reportTypingIndicator: {
isTyping: 'está escribiendo...',
areTyping: 'están escribiendo...',
Expand Down
20 changes: 1 addition & 19 deletions src/libs/actions/Report.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,16 +416,6 @@ function fetchIOUReportByID(iouReportID, chatReportID, shouldRedirectIfEmpty = f
});
}

/**
* @param {Number} reportID
* @param {Number} sequenceNumber
*/
function setNewMarkerPosition(reportID, sequenceNumber) {
Onyx.merge(`${ONYXKEYS.COLLECTION.REPORT}${reportID}`, {
newMarkerSequenceNumber: sequenceNumber,
});
}

/**
* Get the private pusher channel name for a Report.
*
Expand Down Expand Up @@ -684,7 +674,7 @@ function createOptimisticReport(participantList) {
lastActorEmail: '',
lastMessageHtml: '',
lastMessageText: null,
lastReadSequenceNumber: undefined,
lastReadSequenceNumber: 0,
lastMessageTimestamp: 0,
lastVisitedTimestamp: 0,
maxSequenceNumber: 0,
Expand Down Expand Up @@ -1121,7 +1111,6 @@ function markCommentAsUnread(reportID, sequenceNumber) {
onyxMethod: CONST.ONYX.METHOD.MERGE,
key: `${ONYXKEYS.COLLECTION.REPORT}${reportID}`,
value: {
newMarkerSequenceNumber: sequenceNumber,
lastReadSequenceNumber: newLastReadSequenceNumber,
lastVisitedTimestamp: Date.now(),
unreadActionCount: calculateUnreadActionCount(reportID, newLastReadSequenceNumber, maxSequenceNumber),
Expand Down Expand Up @@ -1523,12 +1512,6 @@ function viewNewReportAction(reportID, action) {
return;
}

// When a new message comes in, if the New marker is not already set (newMarkerSequenceNumber === 0), set the marker above the incoming message.
const report = lodashGet(allReports, 'reportID', {});
if (lodashGet(report, 'newMarkerSequenceNumber', 0) === 0 && report.unreadActionCount > 0) {
setNewMarkerPosition(reportID, report.lastReadSequenceNumber + 1);
}

Log.info('[LOCAL_NOTIFICATION] Creating notification');
LocalNotification.showCommentNotification({
reportAction: action,
Expand Down Expand Up @@ -1589,7 +1572,6 @@ export {
addAttachment,
reconnect,
updateNotificationPreference,
setNewMarkerPosition,
subscribeToReportTypingEvents,
subscribeToUserEvents,
subscribeToReportCommentPushNotifications,
Expand Down
3 changes: 0 additions & 3 deletions src/pages/home/ReportScreen.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,6 @@ const propTypes = {
/** The largest sequenceNumber on this report */
maxSequenceNumber: PropTypes.number,

/** The current position of the new marker */
newMarkerSequenceNumber: PropTypes.number,

/** Whether there is an outstanding amount in IOU */
hasOutstandingIOU: PropTypes.bool,

Expand Down
34 changes: 6 additions & 28 deletions src/pages/home/report/FloatingMessageCounter/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,25 +11,17 @@ import withLocalize, {withLocalizePropTypes} from '../../../../components/withLo
import FloatingMessageCounterContainer from './FloatingMessageCounterContainer';

const propTypes = {
/** Count of new messages to show in the badge */
count: PropTypes.number,
/** Whether the New Messages indicator is active */
isActive: PropTypes.bool,

/** Whether the marker is active */
active: PropTypes.bool,

/** Callback to be called when user closes the badge */
onClose: PropTypes.func,

/** Callback to be called when user clicks the marker */
/** Callback to be called when user clicks the New Messages indicator */
onClick: PropTypes.func,

...withLocalizePropTypes,
};

const defaultProps = {
count: 0,
active: false,
onClose: () => {},
isActive: false,
onClick: () => {},
};

Expand All @@ -45,7 +37,7 @@ class FloatingMessageCounter extends PureComponent {
}

componentDidUpdate() {
if (this.props.active && this.props.count > 0) {
if (this.props.isActive) {
this.show();
} else {
this.hide();
Expand Down Expand Up @@ -93,24 +85,10 @@ class FloatingMessageCounter extends PureComponent {
styles.textWhite,
]}
>
{this.props.translate(
'newMessageCount',
{count: this.props.count},
)}
{this.props.translate('newMessages')}
</Text>
</View>
)}
shouldRemoveRightBorderRadius
/>
<Button
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this mean the new messages indicator can't be closed with an X? Why the change? Just curious

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Largely just a matter of simplification and nobody fought to keep it. In the new design, the visibility of the component is enabled based on these conditions alone:

  • The new marker is set
  • The user is scrolled up past a certain distance

Doc here -> https://docs.google.com/document/d/1gvPDhGTxdcu-0lQxvruQnQJfJBrAmSjHieKciucV8eo/edit#bookmark=id.v1smzjvcr8ra

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think it's something we'll miss? Should be relatively easy to add it back in if so. I kinda found it just made the code harder to understand and not really all that useful of a feature.

Copy link
Contributor

Choose a reason for hiding this comment

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

No I like this change actually, this was just my first time looking at this code, and this is a change I "picked up" that I wasn't expecting, and I wanted to make sure I understood it correctly, since I'm still kind of a noob when it comes to reviews in App

success
small
style={[styles.buttonDropdown]}
onPress={this.props.onClose}
shouldRemoveLeftBorderRadius
ContentComponent={() => (
<Icon small src={Expensicons.Close} fill={themeColors.textReversed} />
)}
/>
</View>
</View>
Expand Down
14 changes: 8 additions & 6 deletions src/pages/home/report/ReportActionsList.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ import CONST from '../../../CONST';
import * as StyleUtils from '../../../styles/StyleUtils';

const propTypes = {
/** Position of the "New" line marker */
newMarkerSequenceNumber: PropTypes.number.isRequired,

/** Personal details of all the users */
personalDetails: PropTypes.objectOf(participantPropTypes),

Expand All @@ -30,9 +33,6 @@ const propTypes = {
/** The largest sequenceNumber on this report */
maxSequenceNumber: PropTypes.number,

/** The current position of the new marker */
newMarkerSequenceNumber: PropTypes.number,

/** Whether there is an outstanding amount in IOU */
hasOutstandingIOU: PropTypes.bool,
}).isRequired,
Expand Down Expand Up @@ -134,8 +134,10 @@ class ReportActionsList extends React.Component {
item,
index,
}) {
const shouldDisplayNewIndicator = this.props.report.newMarkerSequenceNumber > 0
&& item.action.sequenceNumber === this.props.report.newMarkerSequenceNumber;
// When the new indicator should not be displayed we explicitly set it to 0. The marker should never be shown above the
// created action (which will have sequenceNumber of 0) so we use 0 to indicate "hidden".
const shouldDisplayNewIndicator = this.props.newMarkerSequenceNumber > 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the first condition needed here? can both sequenceNumber and newMarkerSequenceNumber be equal to zero simultaneously?

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like we explicitly set this to zero, could we set to null to indicate the lack of a value here instead? NAB but since zero could be a valid sequence number (just based on the data type), null could be a little cleaner

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if I understand the question. But the newMarkerSequenceNumber can be equal to zero and the created action has a sequenceNumber of 0. So we do this to avoid showing the marker for the "created" action. We don't want to show the marker when the last read sequenceNumber is 0.

As for using null instead, I don't have strong feelings on it, but maybe we can add a comment here at least to explain what I just mentioned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know if this comment helps. Or if you feel strongly that we should use null. I'm inclined to leave it since everything is working well for me.

Copy link
Contributor

Choose a reason for hiding this comment

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

The comment makes sense to me, so I agree it's 👌 to leave it using 0

&& item.action.sequenceNumber === this.props.newMarkerSequenceNumber;
return (
<ReportActionItem
reportID={this.props.report.reportID}
Expand Down Expand Up @@ -171,7 +173,7 @@ class ReportActionsList extends React.Component {
render() {
// Native mobile does not render updates flatlist the changes even though component did update called.
// To notify there something changes we can use extraData prop to flatlist
const extraData = (!this.props.isDrawerOpen && this.props.isSmallScreenWidth) ? this.props.report.newMarkerSequenceNumber : undefined;
const extraData = (!this.props.isDrawerOpen && this.props.isSmallScreenWidth) ? this.props.newMarkerSequenceNumber : undefined;
const shouldShowReportRecipientLocalTime = ReportUtils.canShowReportRecipientLocalTime(this.props.personalDetails, this.props.report);
return (
<Animated.View style={StyleUtils.getReportListAnimationStyle(this.state.fadeInAnimation)}>
Expand Down
Loading