Simplify new line indicator and message badge #10582

marcaaron · 2022-08-25T21:06:44Z

Details

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/224876

Tests

Start a chat between User A and User B.
Ensure User A is not looking at the chat.
Send a message as User B
Verify the LHN row shows in bold for User A to indicate unread messages
Visit the chat as User A and verify the LHN row no longer shows in bold
Verify there is a “----- New” line above the message sent by User B
Scroll up and verify that the “New Messages” button appears floating above the content.
Verify that tapping this button will scroll you to the bottom of the chat but not clear the “---- New” line marker.
Mark a comment as “unread”
Verify the “---- New” line marker appears
Mark as unread comments above and below the existing new marker
Verify the marker appears in the correct position each time
While the new marker is active leave a comment as User A
Verify the new marker and new messages badge both disappear
Mark another comment as unread.
Navigate away from the chat
Navigate back to the chat
Verify the chat in the LHN shows as read
Verify the New Line marker and new messages badge is still present
Navigate away one more time and back
Verify the new marker and new messages badge both disappear
Mark another comment as unread.
Navigate away from the chat
Navigate back to the chat
Minimize the app and reopen it
Verify the new marker and new message badge both disappear
Switch to a mobile view (small screen) and mark a message as unread
Tap the header to reveal the LHN
Verify the chat in the LHN shows as “unread”
Tap the chat
Verify the new marker and badge are present
Tap the header to reveal the LHN
Verify the chat in the LHN shows as “read”
Tap the chat
Verify the new marker and badge are no longer present.
Scroll up in the chat so that last comment is out of view
Leave a comment as User B
Verify the “new messages badge” shows for User A
Scroll down and verify the “new line marker” shows for User A in the correct position.
Scroll back up and leave another comment as User B
Scroll down as User A
Verify the new marker is in the same position as it was before

Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed

QA Steps

Same as Test Steps

Verify that no errors appear in the JS console

Screenshots

Web

Mobile Web

Desktop

iOS

Android

marcaaron · 2022-08-26T17:20:13Z

This PR is required to test atm as I discovered a regression caused by the OpenReport changes.

marcaaron · 2022-08-29T09:37:34Z

This is still on HOLD but testing well so marking it as ready.

cead22 · 2022-08-29T10:38:30Z

src/pages/home/report/FloatingMessageCounter/index.js

@@ -11,14 +11,8 @@ 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 marker is active */


Suggested change

/** Whether the marker is active */

/** Whether the New Messages indicator is active */

Should we rename this file since it's no longer showing counts?

Missed the comment below this but going to raise a separate PR to clean up the file naming

cead22 · 2022-08-29T10:39:19Z

src/pages/home/report/FloatingMessageCounter/index.js

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

    /** Callback to be called when user clicks the marker */


Suggested change

/** Callback to be called when user clicks the marker */

/** Callback to be called when user clicks the New Messages indicator */

Not sure if these comments were copy paste from the new messages marker, or if we're calling both things a marker, but I think it's good to differentiate for clarity

cead22 · 2022-08-29T10:41:03Z

src/pages/home/report/FloatingMessageCounter/index.js

                                    </Text>
                                </View>
                            )}
-                            shouldRemoveRightBorderRadius
-                        />
-                        <Button


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

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

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.

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

cead22 · 2022-08-29T10:42:32Z

src/pages/home/report/ReportActionsList.js

@@ -134,8 +134,8 @@ class ReportActionsList extends React.Component {
        item,
        index,
    }) {
-        const shouldDisplayNewIndicator = this.props.report.newMarkerSequenceNumber > 0
-            && item.action.sequenceNumber === this.props.report.newMarkerSequenceNumber;
+        const shouldDisplayNewIndicator = this.props.newMarkerSequenceNumber > 0


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

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

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.

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.

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

cead22 · 2022-08-29T10:48:36Z

src/pages/home/report/ReportActionsView.js

@@ -174,53 +176,79 @@ class ReportActionsView extends React.Component {
    }

    componentDidUpdate(prevProps) {
+        const isReportFullyVisible = this.getIsReportFullyVisible();
+
+        // NETWORK CHANGE DETECTED


Not a fan of these capitalized comments, can we make them lowercase and keep consistency with all other comments, or use variable to communicate this? Eg,

const bool wasNetworkChangeDetected = lodashGet(prevProps.network, 'isOffline') && !lodashGet(this.props.network, 'isOffline'); ... if (wasNetworkChangeDetected) { ...

With the comments I wanted to create a sense that these are discrete "events" or "situations" we might encounter.

I had a draft where each of these was moved to a method like checkIfNetworkChangedAndRefetch(), but that felt like something Uncle Bob would do and maybe overkill. I eventually just decided to do something obvious and ugly. Will try the variable thing! Thanks!

Making these comments not be all uppercase already solves my concern. I think we should follow our guidelines, so others can as well. If we break consistency for our comments, contributors may start doing the same

And to be clear, I did appreciate the comments that are in this code, as it would've been much harder to follow without them

cead22 · 2022-08-29T10:53:24Z

src/pages/home/report/ReportActionsView.js

+        // REPORT PREVIOUSLY HIDDEN BY SIDEBAR/LHN OR VIEW EXPANDED FROM MOBILE TO DESKTOP LAYOUT
+        // We update the new marker position, mark the report as read, and fetch new report actions


Suggested change

// REPORT PREVIOUSLY HIDDEN BY SIDEBAR/LHN OR VIEW EXPANDED FROM MOBILE TO DESKTOP LAYOUT

// We update the new marker position, mark the report as read, and fetch new report actions

// If the report was previously hidden by the side bar, or the view is expanded from mobile to desktop layout

// we update the new marker position, mark the report as read, and fetch new report actions

cead22 · 2022-08-29T10:55:57Z

src/pages/home/report/ReportActionsView.js

            Report.openReport(this.props.report.reportID);
        }
+
+        // REPORT PREVIOUSLY OPEN BUT USER NAVIGATED TO SIDEBAR/LHN
+        // When the user swipes back on the report or navigates to the LHN the ReportActionsView doesn’t unmount and just remains hidden.


How's "swipes back on the report" different from "navigates to the LHN"?

Suggested change

// When the user swipes back on the report or navigates to the LHN the ReportActionsView doesn’t unmount and just remains hidden.

// When the user swipes back on the report or navigates to the LHN the ReportActionsView doesn't unmount and just remains hidden.

I'll be more general here. I meant swipe or tap the back button. Both would be ways of navigating to the LHN.

cead22 · 2022-08-29T10:56:04Z

src/pages/home/report/ReportActionsView.js

            Report.openReport(this.props.report.reportID);
        }
+
+        // REPORT PREVIOUSLY OPEN BUT USER NAVIGATED TO SIDEBAR/LHN


Suggested change

// REPORT PREVIOUSLY OPEN BUT USER NAVIGATED TO SIDEBAR/LHN

cead22 · 2022-08-29T10:56:37Z

src/pages/home/report/ReportActionsView.js

+        }
+
+        // REPORT COMMENT MANUALLY MARKED AS UNREAD
+        // Checks to see if a report comment has been manually "marked as read". All other times when the lastReadSequenceNumber


Suggested change

// Checks to see if a report comment has been manually "marked as read". All other times when the lastReadSequenceNumber

// Checks to see if a report comment has been manually "marked as unread". All other times when the lastReadSequenceNumber

cead22 · 2022-08-29T10:56:41Z

src/pages/home/report/ReportActionsView.js

+            this.setState({newMarkerSequenceNumber: 0});
+        }
+
+        // REPORT COMMENT MANUALLY MARKED AS UNREAD


Suggested change

// REPORT COMMENT MANUALLY MARKED AS UNREAD

amyevans

I don't have a lot of context for these changes, but they test well for me locally, and the logic and comments make sense to me

amyevans · 2022-08-30T07:55:48Z

src/pages/home/report/ReportActionsList.js

@@ -134,8 +134,8 @@ class ReportActionsList extends React.Component {
        item,
        index,
    }) {
-        const shouldDisplayNewIndicator = this.props.report.newMarkerSequenceNumber > 0
-            && item.action.sequenceNumber === this.props.report.newMarkerSequenceNumber;
+        const shouldDisplayNewIndicator = this.props.newMarkerSequenceNumber > 0


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

OSBotify · 2022-08-31T13:25:41Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2022-09-01T14:25:20Z

🚀 Deployed to staging by @marcaaron in version: 1.1.96-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

roryabraham · 2022-09-02T08:03:21Z

Going to try reverting this to see if it's the cause for #10751

marcaaron added 2 commits August 25, 2022 20:55

Fix the new line marker and floating message indicator

0107ad3

Fix issues

c78fd56

marcaaron self-assigned this Aug 25, 2022

marcaaron added 3 commits August 25, 2022 23:08

remove logs

ebd4ac2

remove outdated comment

cc2a820

Improve documentation

7451e17

marcaaron changed the title ~~[WIP] Simplify new line indicator and message badge~~ [HOLD Web-Expensify/pull/34696] Simplify new line indicator and message badge Aug 26, 2022

marcaaron changed the title ~~[HOLD Web-Expensify/pull/34696] Simplify new line indicator and message badge~~ [HOLD Web-Expensify 34696] Simplify new line indicator and message badge Aug 26, 2022

marcaaron marked this pull request as ready for review August 29, 2022 09:37

marcaaron requested a review from a team as a code owner August 29, 2022 09:37

melvin-bot bot requested review from amyevans and removed request for a team August 29, 2022 09:38

marcaaron requested a review from sketchydroide August 29, 2022 09:51

cead22 requested changes Aug 29, 2022

View reviewed changes

marcaaron added 2 commits August 29, 2022 13:42

improve comments in componentDidUpdate

64eed1c

Update comment

8b72671

cead22 approved these changes Aug 29, 2022

View reviewed changes

amyevans approved these changes Aug 30, 2022

View reviewed changes

marcaaron changed the title ~~[HOLD Web-Expensify 34696] Simplify new line indicator and message badge~~ Simplify new line indicator and message badge Aug 31, 2022

marcaaron merged commit bc57fff into main Aug 31, 2022

marcaaron deleted the marcaaron-simplifyNewLineAndCounter branch August 31, 2022 13:20

OSBotify mentioned this pull request Sep 1, 2022

Deploy Checklist: New Expensify 2022-09-01 #10748

Closed

24 tasks

kbecciv mentioned this pull request Sep 1, 2022

[HOLD for payment 2022-10-31] [Bug] [$250] Desktop - Chat- New marker doesn't disappear after App minimize/restore #10767

Closed

roryabraham mentioned this pull request Sep 2, 2022

Revert "Simplify new line indicator and message badge" #10776

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify new line indicator and message badge #10582

Simplify new line indicator and message badge #10582

marcaaron commented Aug 25, 2022 •

edited

Loading

marcaaron commented Aug 26, 2022

marcaaron commented Aug 29, 2022

cead22 Aug 29, 2022

marcaaron Aug 29, 2022

cead22 Aug 29, 2022

cead22 Aug 29, 2022

marcaaron Aug 29, 2022

marcaaron Aug 29, 2022

cead22 Aug 29, 2022

cead22 Aug 29, 2022

cead22 Aug 29, 2022

marcaaron Aug 29, 2022

marcaaron Aug 29, 2022

amyevans Aug 30, 2022

cead22 Aug 29, 2022

marcaaron Aug 29, 2022

cead22 Aug 29, 2022

cead22 Aug 29, 2022

cead22 Aug 29, 2022

cead22 Aug 29, 2022

marcaaron Aug 29, 2022

cead22 Aug 29, 2022

cead22 Aug 29, 2022

cead22 Aug 29, 2022

amyevans left a comment

amyevans Aug 30, 2022

OSBotify commented Aug 31, 2022

OSBotify commented Sep 1, 2022

roryabraham commented Sep 2, 2022

	/** Whether the marker is active */
	/** Whether the New Messages indicator is active */

	/** Callback to be called when user clicks the marker */
	/** Callback to be called when user clicks the New Messages indicator */

		// REPORT PREVIOUSLY HIDDEN BY SIDEBAR/LHN OR VIEW EXPANDED FROM MOBILE TO DESKTOP LAYOUT
		// We update the new marker position, mark the report as read, and fetch new report actions

	// When the user swipes back on the report or navigates to the LHN the ReportActionsView doesn’t unmount and just remains hidden.
	// When the user swipes back on the report or navigates to the LHN the ReportActionsView doesn't unmount and just remains hidden.

	// Checks to see if a report comment has been manually "marked as read". All other times when the lastReadSequenceNumber
	// Checks to see if a report comment has been manually "marked as unread". All other times when the lastReadSequenceNumber

Simplify new line indicator and message badge #10582

Simplify new line indicator and message badge #10582

Conversation

marcaaron commented Aug 25, 2022 • edited Loading

Details

Fixed Issues

Tests

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Screenshots

Web

Mobile Web

Desktop

iOS

Android

marcaaron commented Aug 26, 2022

marcaaron commented Aug 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amyevans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OSBotify commented Aug 31, 2022

OSBotify commented Sep 1, 2022

roryabraham commented Sep 2, 2022

marcaaron commented Aug 25, 2022 •

edited

Loading