[$250] Update the behavior of the yellow thread highlight after exiting a thread

sonialiap commented 5 months ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Version Number: Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: n/a Email or phone of affected tester (no customers): everyone Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: @JmillsExpensify Slack conversation: https://expensify.slack.com/archives/C03U7DCU4/p1714062455602839

Action Performed:

Click into a thread
Click back into the main conversation by clicking the the room title in the top left of the thread window
Observe that the thread is highlighted in yellow after you exit it
Expected Result:

The yellow highlight should fade after
- Clicking out of the chat into another chat
- Clicking into another thread
- Refreshing the page
- Sending a new comment in the same chat (but not receiving a comment from another user)
- Taking any action from the + menu while in the chat

Actual Result:

Highlight remains until you click out of the chat into another chat. It does not clear after any of the other cases mentioned above.

Workaround:

Usable but many have found the highlight confusing and distracting

Platforms:

Which of our officially supported platforms is this issue occurring on?

[ ] Android: Native
[ ] Android: mWeb Chrome
[ ] iOS: Native
[ ] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[ ] MacOS: Desktop

Screenshots/Videos

https://github.com/Expensify/App/assets/17131195/aa847e2d-cda1-40ba-a825-f3e1affc5129

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~019325e864a40fa38f
Upwork Job ID: 1790455576916172800
Last Price Increase: 2024-05-14
Automatic offers:

brunovjk | Contributor | 102439671

Issue Owner

Current Issue Owner: @sobitneupane

melvin-bot[bot] commented 5 months ago

Triggered auto assignment to @lschurr (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

melvin-bot[bot] commented 5 months ago

Job added to Upwork: https://www.upwork.com/jobs/~019325e864a40fa38f

melvin-bot[bot] commented 5 months ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @sobitneupane (External)

brunovjk commented 5 months ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

The yellow thread highlight in the Expensify app persists even after the user has exited the thread and performed other actions within the same chat.

What is the root cause of that problem?

The root cause lies in the highlightedBackgroundColorIfNeeded logic within ReportActionItem.tsx. Currently, the highlight is only cleared when the user navigates to a different chat. It does not consider other actions that should logically dismiss the highlight.

What changes do you think we should make in order to solve the problem?

We should modify the highlightedBackgroundColorIfNeeded logic to include additional conditions for clearing the highlight. Specifically, the highlight should be cleared when:

The user clicks out of the chat into another chat.
The user clicks into another thread within the same chat.
The user refreshes the page.
The user sends a new comment in the same chat.
The user takes any action from the "+" menu while in the chat.

To achieve this, we can introduce a state variable (e.g., isThreadHighlighted) and a useEffect hook to manage the highlight state. The hook would listen for the relevant events (e.g., navigation changes, new comments, "+" menu actions) and update the isThreadHighlighted state accordingly. The highlightedBackgroundColorIfNeeded logic would then depend on this state variable to determine whether to apply the highlight.

Details on Events and Handling:

1. Navigation Changes (Switching Chats):

How to Listen: Utilize the useNavigation hook from React Navigation to detect changes in the active chat route.
How to Handle: When the active chat route changes, set isThreadHighlighted to false.

2. Clicking into another thread within the same chat:

How to Listen: Use the onPress prop of the ReportActionItem component to detect when a different thread is clicked within the same chat.
How to Handle: In the onPress handler, check if the clicked report action is part of a different thread. If so, set isThreadHighlighted to false.

3. Sending a New Comment in the Same Chat:

How to Listen: Since ReportActionItem does not have direct access to the comment submission event, a potential solution is to lift the state up to a common parent component (e.g., ReportActionsList). This parent component could then pass down a callback function to ReportActionItem to be triggered when a new comment is submitted successfully.
How to Handle: The callback function passed down to ReportActionItem would set the isThreadHighlighted to false.

4. Taking Any Action from the "+" Menu While in the Chat:

How to Listen: In the ReportActionCompose component, the onAddActionPressed function seems to be a good candidate to trigger an event when the "+" menu is pressed. We could either create a custom event or use a context to communicate this event to the ReportActionItem component.
How to Handle: The ReportActionItem component would listen for the event (using useEffect or context) and set isThreadHighlighted to false when the event is triggered.

What alternative solutions did you explore? (Optional)

One alternative solution could be to use a timer to automatically clear the highlight after a certain delay. However, this approach might not be ideal as it could lead to inconsistent behavior depending on the user's actions. Another alternative is to clear the highlight whenever the user interacts with any element outside the highlighted thread. This might be too broad and could unintentionally clear the highlight in some cases.

nkdengineer commented 5 months ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Highlight remains until you click out of the chat into another chat. It does not clear after any of the other cases mentioned above.

What is the root cause of that problem?

The report action is always highlighted if it's the linked action.

https://github.com/Expensify/App/blob/edd1719f62d74663d52f4168db2fd7adfe0cdda3/src/pages/home/report/ReportActionItem.tsx#L215

What changes do you think we should make in order to solve the problem?

I think we can do the same behavior in Slack that only highlights the link action in a few seconds so we don't need to take care what action we should clear the highlighted action

We can do this by using animated view here

https://github.com/Expensify/App/blob/edd1719f62d74663d52f4168db2fd7adfe0cdda3/src/pages/home/report/ReportActionItem.tsx#L951

Create a shared value

const bgColor = useSharedValue<string>(isReportActionLinked ? theme.messageHighlightBG : 'inherit');

Create a useEffect to change the value to inherit

useEffect(() => {
    if (!isReportActionLinked) {
        return;
    }

    bgColor.value = withTiming('inherit', {
        duration: 3000,
        easing: Easing.inOut(Easing.ease),
    })
}, [isReportActionLinked, theme.messageHighlightBG])

Create a useEffect to do the animation again if the page is changed from un-focused to focused

const isFocused = useIsFocused();
const prevIsFocused = usePrevious(isFocused);
useEffect(() => {
    if (!isFocused || prevIsFocused || !isReportActionLinked) {
        return;
    }

    bgColor.value = theme.messageHighlightBG;
    bgColor.value = withTiming('inherit', {
        duration: 3000,
        easing: Easing.inOut(Easing.ease),
    })
}, [isFocused, prevIsFocused, isReportActionLinked])

Change the style here to use animated style

const highlightedBackgroundColorIfNeeded = useAnimatedStyle(() =>
    StyleUtils.getBackgroundColorStyle(bgColor.value)
);

https://github.com/nkdengineer/App/blob/edd1719f62d74663d52f4168db2fd7adfe0cdda3/src/pages/home/report/ReportActionItem.tsx#L215

Use animated view here

<Animated.View style={highlightedBackgroundColorIfNeeded}>

https://github.com/Expensify/App/blob/edd1719f62d74663d52f4168db2fd7adfe0cdda3/src/pages/home/report/ReportActionItem.tsx#L951

What alternative solutions did you explore? (Optional)

NA

Result

https://github.com/Expensify/App/assets/161821005/203242b6-ba90-498d-b35f-697a96a75969

lschurr commented 5 months ago

Hi @sobitneupane - Can you take a look at these proposals?

sobitneupane commented 5 months ago

Thanks for the proposal @brunovjk

The hook would listen for the relevant events (e.g., navigation changes, new comments, "+" menu actions) and update the isThreadHighlighted state accordingly.

Can you please add more details about each events that hook would listen to and how will you handle it?

brunovjk commented 5 months ago

Thanks for the review @sobitneupane

Proposal

Updated

Added more detailed implementation steps for handling events that should clear the thread highlight

sobitneupane commented 5 months ago

Thanks for the update @brunovjk

Proposal from @brunovjk looks good to me.

🎀 👀 🎀 C+ reviewed

melvin-bot[bot] commented 5 months ago

Triggered auto assignment to @yuwenmemon, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

nkdengineer commented 5 months ago

Clicking out of the chat into another chat Clicking into another thread Refreshing the page Sending a new comment in the same chat (but not receiving a comment from another user) Taking any action from the + menu while in the chat

@sobitneupane I think we shouldn't removing the highlight of a message when we do these action because in addition to those, we have many other actions that should also remove the highlight like edit another comment, react, ... Instead, we should only have an animation for linked report action like Slack so we don't need to care what action we should clear the highlight. What do you think?

cc @yuwenmemon

sobitneupane commented 5 months ago

I think we shouldn't removing the highlight of a message when we do these action because in addition to those, we have many other actions that should also remove the highlight like edit another comment, react, ... Instead, we should only have an animation for linked report action like Slack so we don't need to care what action we should clear the highlight.

That's a good point @nkdengineer. I kind of agree with you. @yuwenmemon What's your take on highlighting just for few seconds and fading it out?

brunovjk commented 5 months ago

I also think it makes sense, I even pointed out this solution in the "alternative solutions" section before. We just need to test it carefully, but I think it will be good too.

brunovjk commented 5 months ago

@sobitneupane, @yuwenmemon and @nkdengineer What do you think about using both options together? It disappeared after a while and/or when the user performed some action.

yuwenmemon commented 5 months ago

Hmmm... I'd defer to @Expensify/design on this question.

The question is: Should the "yellow highlight" that highlights a linked message disappear a few seconds after showing?

shawnborton commented 5 months ago

I think we want to do what is outlined in the original comment:

The yellow highlight should fade after

Clicking out of the chat into another chat Clicking into another thread Refreshing the page Sending a new comment in the same chat (but not receiving a comment from another user) Taking any action from the + menu while in the chat

cc @sonialiap for confirmation though. I know this one was a long discussion, and I'm fairly certain we landed on the scenarios listed above.

dannymcclain commented 5 months ago

The question is: Should the "yellow highlight" that highlights a linked message disappear a few seconds after showing?

This has come up several times in Slack and there's been quite a bit of discussion about it. We've consistently received push-back about the highlight fading after a set period of time (some people are for it, some people are against it), and I think the consensus was to rely on actions to dismiss it rather than time. This seemed to be the best compromise amongst the entire team, so I think we should continue to pursue that path.

shawnborton commented 5 months ago

Exactly ^ 🤣

brunovjk commented 5 months ago

Awesome! @yuwenmemon Can I move forward with my proposal?

yuwenmemon commented 5 months ago

Yep

melvin-bot[bot] commented 5 months ago

📣 @brunovjk 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

brunovjk commented 5 months ago

ETA 05/30

melvin-bot[bot] commented 4 months ago

@yuwenmemon @sobitneupane @lschurr @brunovjk this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

brunovjk commented 4 months ago

Not overdue. Continue working in PR

melvin-bot[bot] commented 4 months ago

@yuwenmemon, @sobitneupane, @lschurr, @brunovjk Huh... This is 4 days overdue. Who can take care of this?

lschurr commented 4 months ago

Go away Melv - https://github.com/Expensify/App/issues/42165#issuecomment-2135829854

brunovjk commented 4 months ago

@yuwenmemon and @sobitneupane I found the task more challenging than expected. ETA might need to extend. Will keep working and raise the PR ASAP. Is this okay? Thank you.

yuwenmemon commented 4 months ago

All good

brunovjk commented 4 months ago

Update: Keep working on the PR, I will raise it ASAP

lschurr commented 4 months ago

Any update @brunovjk?

brunovjk commented 4 months ago

Sorry for the delay, I had to focus on another issue but I'm back here, I just need to do a few tests and we're ready :D

brunovjk commented 4 months ago

Update: Keep working on the PR

brunovjk commented 4 months ago

Update: Same as above

brunovjk commented 4 months ago

Update: I got almost all cover:

[x] - Clicking out of the chat into another chat
[x] - Clicking into another thread
[ ] - Refreshing the page
[x] - Sending a new comment in the same chat (but not receiving a comment from another user)
[x] - Taking any action from the + menu while in the chat

I need to confirm the expected behavior for the highlight when refreshing the page. According to the previous PR that introduced the highlight for a linked report, a report item should be highlighted when a user sends a link (e.g., 456456456456/r/465456456464) to another user. However, if the user refreshes the page using the same link, should the highlight still be removed? Thank you.

cc: @sobitneupane @shawnborton @dannymcclain @yuwenmemon

dannymcclain commented 4 months ago

However, if the user refreshes the page using the same link, should the highlight still be removed?

I believe this is the desired behavior—remove the highlight if you refresh the page. I'll let others chime in too to make sure I've followed the conversation correctly.

melvin-bot[bot] commented 4 months ago

@yuwenmemon @sobitneupane @lschurr @brunovjk this issue is now 4 weeks old, please consider:

Finding a contributor to fix the bug
Closing the issue if BZ has been unable to add the issue to a VIP or Wave project
If you have any questions, don't hesitate to start a discussion in #expensify-open-source

Thanks!

brunovjk commented 4 months ago

Melvin PR is almost ready

brunovjk commented 4 months ago

Update: Still need to safely implement the dismissal when the user refreshes the page, without crashing the new 'Comment Linking' feature.

brunovjk commented 4 months ago

Update: Testing on multiple platforms. Next: Record videos and raise PR.

brunovjk commented 4 months ago

Update: Same as above

brunovjk commented 4 months ago

Update: Continue working on PR.

brunovjk commented 3 months ago

Update: Same as above

brunovjk commented 3 months ago

Update: The feat to reset the yellow highlight when refreshing a page is taking a little more work than expected, but going well, get back with the PR to review ASAP

brunovjk commented 3 months ago

Update: Same as above

brunovjk commented 2 months ago

Update: Continue working on PR. Catching possible regressions.

sobitneupane commented 2 months ago

@shawnborton @yuwenmemon Should we remove the yellow highlight as soon as we begin taking any action (for example, after clicking Submit expense from the + menu), or after the action is complete?

shawnborton commented 2 months ago

Probably as soon as we begin taking any action? Curious what @Expensify/design and @yuwenmemon thinks though.

dannymcclain commented 2 months ago

Probably as soon as we begin taking any action?

Makes sense to me.

dubielzyk-expensify commented 2 months ago

Agree with the designers

brunovjk commented 2 months ago

Update: PR in review

Expensify / App