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

Introduce "Mark as unread" functionality in Expensify.cash #2433

Merged
merged 23 commits into from
May 5, 2021

Conversation

Gonals
Copy link
Contributor

@Gonals Gonals commented Apr 16, 2021

@tgolen @marcaaron @roryabraham, this is the new PR!

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/147475

Tests

  1. Log into e.cash and open a conversation with existing messages
  2. On the hover menu of one of the messages, select "mark as unread":

Screen Shot 2021-03-16 at 3 43 38 PM

3. The `new` marker should show up over that message and the chat in the LHN should become bolded:

Screen Shot 2021-03-16 at 3 43 54 PM

  1. Switch chats. It should remain bolded
  2. Switch back to the original chat. The new marker should be correctly in place and the chat in the LHN should unbold instantly. The new marker should remain in place after that.
  3. Switch out and back in. the marker should be gone.
  4. Do some regression testing to make sure the new marker shows correctly for actual new messages
  5. Test marking a message as unread and receiving new messages afterwards. The new marker should remain in the correct message.
  6. Test marking a message as unread, leaving the chat, and then receiving new messages afterwards. The new marker should remain in the marked message.

Tested On

Testing on mobile soon (I pretty much destroyed my repository, so it is being a pain to get it to work in mobile again)!

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Desktop

Screen Shot 2021-04-23 at 11 10 09 AM

iOS

Screen Shot 2021-04-23 at 10 33 44 AM

Screen Shot 2021-04-23 at 10 33 50 AM

Screen Shot 2021-04-23 at 10 33 29 AM

Android

Screen Shot 2021-04-23 at 10 50 25 AM

Screen Shot 2021-04-23 at 10 50 36 AM

@Gonals Gonals requested a review from a team as a code owner April 16, 2021 13:25
@Gonals Gonals requested a review from marcaaron April 16, 2021 13:25
@MelvinBot MelvinBot requested review from nkuoch and removed request for a team April 16, 2021 13:25
src/libs/actions/Report.js Outdated Show resolved Hide resolved
@Gonals Gonals self-assigned this Apr 19, 2021
@Gonals Gonals changed the title [WIP] Introduce "Mark as unread" functionality in Expensify.cash Introduce "Mark as unread" functionality in Expensify.cash Apr 20, 2021
@Gonals Gonals changed the title Introduce "Mark as unread" functionality in Expensify.cash [WIP] Introduce "Mark as unread" functionality in Expensify.cash Apr 20, 2021
@Gonals Gonals changed the title [WIP] Introduce "Mark as unread" functionality in Expensify.cash Introduce "Mark as unread" functionality in Expensify.cash Apr 20, 2021
@Gonals Gonals requested review from tgolen and roryabraham and removed request for nkuoch April 20, 2021 14:35
src/libs/actions/Report.js Outdated Show resolved Hide resolved
src/pages/home/report/ReportActionContextMenu.js Outdated Show resolved Hide resolved
src/pages/home/report/ReportActionsView.js Outdated Show resolved Hide resolved
src/pages/home/report/ReportActionContextMenu.js Outdated Show resolved Hide resolved
// `ReportActionContextMenuItem` with `successText` and `successIcon` which will fallback to
// the `text` and `icon`
onPress: () => {
const message = _.last(lodashGet(this.props.reportAction, 'message', null));
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this should be a class method so that it prevents this constructor from turning into a monolithic block of logic and functions.

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 see this is being discussed in the delete comments PR, so I'll wait for the resolution over there

src/pages/home/report/ReportActionContextMenu.js Outdated Show resolved Hide resolved
src/pages/home/report/ReportActionContextMenu.js Outdated Show resolved Hide resolved
src/pages/home/report/ReportActionContextMenu.js Outdated Show resolved Hide resolved
src/pages/home/report/ReportActionsView.js Outdated Show resolved Hide resolved
src/libs/actions/Report.js Outdated Show resolved Hide resolved
src/libs/actions/Report.js Outdated Show resolved Hide resolved
src/libs/actions/Report.js Outdated Show resolved Hide resolved
src/libs/actions/Report.js Outdated Show resolved Hide resolved
src/libs/actions/Report.js Outdated Show resolved Hide resolved
src/libs/actions/Report.js Outdated Show resolved Hide resolved
src/libs/actions/Report.js Show resolved Hide resolved
src/libs/actions/Report.js Outdated Show resolved Hide resolved
@Gonals
Copy link
Contributor Author

Gonals commented Apr 21, 2021

Comments addressed!

@Gonals
Copy link
Contributor Author

Gonals commented May 3, 2021

All addressed!

tgolen
tgolen previously approved these changes May 3, 2021
Copy link
Contributor

@tgolen tgolen left a comment

Choose a reason for hiding this comment

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

I think this looks much better and the comments do a good job of explaining the +/- 1s that are left.

marcaaron
marcaaron previously approved these changes May 3, 2021
Copy link
Contributor

@marcaaron marcaaron left a comment

Choose a reason for hiding this comment

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

Yes changes are looking much better to me thanks @Gonals !!

@Gonals
Copy link
Contributor Author

Gonals commented May 4, 2021

All yours, @roryabraham

@marcaaron
Copy link
Contributor

Forgot to mention that I came across this in testing. I'm not sure if it's a blocker but if a chat has an unreadActionCount of 0 unread we can see the unread marker above the very first chat message

Screen Shot 2021-05-04 at 12 29 01 PM

Also, we should update the test steps to remove the part about waiting for 3 seconds since the change looks to happen instantly now and QA will ask if it's expected or not.

@Gonals Gonals dismissed stale reviews from marcaaron and tgolen via 97869a4 May 5, 2021 08:23
@Gonals
Copy link
Contributor Author

Gonals commented May 5, 2021

Forgot to mention that I came across this in testing. I'm not sure if it's a blocker but if a chat has an unreadActionCount of 0 unread we can see the unread marker above the very first chat message

Screen Shot 2021-05-04 at 12 29 01 PM

Also, we should update the test steps to remove the part about waiting for 3 seconds since the change looks to happen instantly now and QA will ask if it's expected or not.

Ah! Nice catch! Fixed

@Gonals
Copy link
Contributor Author

Gonals commented May 5, 2021

Made a small fix, so this needs re-approval @marcaaron, @tgolen, @roryabraham

Thanks!

tgolen
tgolen previously approved these changes May 5, 2021
// first unread sequence. If there are no unread actions, there's no need to display it.
if (lastReadSequenceNumber > 0) {
setNewMarkerPosition(this.props.reportID, lastReadSequenceNumber + 1);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If there are no unread actions, there's no need to display it.

So should we set it to 0 instead of 0 + 1 ?

This change didn't resolve the issue I was seeing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are completely right. Should be fixed now. I moved the +1 upwards so we only add it when != 0

@Gonals
Copy link
Contributor Author

Gonals commented May 5, 2021

All comments addressed again!

Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

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

This is looking great!

@marcaaron marcaaron merged commit 5a04fa0 into main May 5, 2021
@marcaaron marcaaron deleted the alberto-markAsUnread branch May 5, 2021 17:28
@Gonals
Copy link
Contributor Author

Gonals commented May 5, 2021

It... merged 😮 ✨

@marcaaron
Copy link
Contributor

WE 👏 DID 👏 IT 👏

@Gonals
Copy link
Contributor Author

Gonals commented May 5, 2021

I'm going to cry 🤣

@tgolen
Copy link
Contributor

tgolen commented May 5, 2021

Wait, I'm confused about the +1. Can we revert this?

@tgolen
Copy link
Contributor

tgolen commented May 5, 2021

:trollface: j/k

@OSBotify
Copy link
Contributor

OSBotify commented May 5, 2021

🚀 Deployed to staging in version: 1.0.37-2🚀

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

@OSBotify
Copy link
Contributor

OSBotify commented May 8, 2021

🚀 Deployed to production in version: 1.0.39-5🚀

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants