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

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

Open
1 of 6 tasks
sonialiap opened this issue May 14, 2024 · 92 comments
Open
1 of 6 tasks
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@sonialiap
Copy link
Contributor

sonialiap commented May 14, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] 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:

  1. Click into a thread
  2. Click back into the main conversation by clicking the the room title in the top left of the thread window
  3. 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
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Screen.Recording.2024-04-26.at.1.50.50.PM.mov
image

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
    • ishpaul777 | Contributor | 104579107
Issue OwnerCurrent Issue Owner: @sobitneupane
@sonialiap sonialiap added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels May 14, 2024
Copy link

melvin-bot bot commented May 14, 2024

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.

@sonialiap sonialiap changed the title Update to the yellow highlight when existing a thread Update the behavior of the yellow thread highlight after exiting a thread May 14, 2024
@JmillsExpensify JmillsExpensify moved this from Polish to HOT PICKS in [#whatsnext] #wave-collect May 14, 2024
@trjExpensify trjExpensify added the External Added to denote the issue can be worked on by a contributor label May 14, 2024
@melvin-bot melvin-bot bot changed the title Update the behavior of the yellow thread highlight after exiting a thread [$250] Update the behavior of the yellow thread highlight after exiting a thread May 14, 2024
Copy link

melvin-bot bot commented May 14, 2024

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

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label May 14, 2024
Copy link

melvin-bot bot commented May 14, 2024

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

@trjExpensify trjExpensify moved this from HOT PICKS to Polish in [#whatsnext] #wave-collect May 14, 2024
@brunovjk
Copy link
Contributor

brunovjk commented May 14, 2024

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
Copy link
Contributor

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.

const highlightedBackgroundColorIfNeeded = useMemo(

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

<View style={highlightedBackgroundColorIfNeeded}>

  1. Create a shared value
const bgColor = useSharedValue<string>(isReportActionLinked ? theme.messageHighlightBG : 'inherit');
  1. 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])
  1. 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])
  1. 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

  1. Use animated view here
<Animated.View style={highlightedBackgroundColorIfNeeded}>

<View style={highlightedBackgroundColorIfNeeded}>

What alternative solutions did you explore? (Optional)

NA

Result

Screen.Recording.2024-05-15.at.11.09.22.mov

@lschurr
Copy link
Contributor

lschurr commented May 15, 2024

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

@sobitneupane
Copy link
Contributor

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
Copy link
Contributor

Thanks for the review @sobitneupane

Proposal

Updated

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

@sobitneupane
Copy link
Contributor

Thanks for the update @brunovjk

Proposal from @brunovjk looks good to me.

🎀 👀 🎀 C+ reviewed

Copy link

melvin-bot bot commented May 21, 2024

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

@nkdengineer
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

brunovjk commented May 23, 2024

@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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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.

@sobitneupane
Copy link
Contributor

Thanks @brunovjk . I appreciate your understanding.

Proposal from @ishpaul777 looks good to me. I believe it will make the thread highlight behavior more consistent and intuitive.

cc: @yuwenmemon

@melvin-bot melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 24, 2024
Copy link

melvin-bot bot commented Oct 24, 2024

📣 @ishpaul777 🎉 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 📖

@ishpaul777
Copy link
Contributor

will create a PR over weekend

@ishpaul777
Copy link
Contributor

PR is ready for review ^

@ishpaul777
Copy link
Contributor

PR is still under review

@twisterdotcom twisterdotcom removed the Bug Something is broken. Auto assigns a BugZero manager. label Nov 18, 2024
@twisterdotcom
Copy link
Contributor

Relabelling as Lauren is OOO now.

@twisterdotcom twisterdotcom added the Bug Something is broken. Auto assigns a BugZero manager. label Nov 18, 2024
Copy link

melvin-bot bot commented Nov 18, 2024

Triggered auto assignment to @strepanier03 (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 melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Nov 18, 2024
Copy link

melvin-bot bot commented Nov 26, 2024

@yuwenmemon, @strepanier03, @sobitneupane, @lschurr, @ishpaul777 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

@ishpaul777
Copy link
Contributor

PR still WIP

Copy link

melvin-bot bot commented Nov 28, 2024

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

@ishpaul777
Copy link
Contributor

PR still WIP

Copy link

melvin-bot bot commented Dec 2, 2024

@yuwenmemon, @strepanier03, @sobitneupane, @lschurr, @ishpaul777 Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

@ishpaul777
Copy link
Contributor

still WIP, @strepanier03 can we move this weekly i expect to finish by EOW

Copy link

melvin-bot bot commented Dec 4, 2024

@yuwenmemon, @strepanier03, @sobitneupane, @lschurr, @ishpaul777 10 days overdue. Is anyone even seeing these? Hello?

Copy link

melvin-bot bot commented Dec 6, 2024

@yuwenmemon, @strepanier03, @sobitneupane, @lschurr, @ishpaul777 12 days overdue now... This issue's end is nigh!

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Dec 9, 2024
Copy link

melvin-bot bot commented Dec 9, 2024

This issue has not been updated in over 14 days. @yuwenmemon, @strepanier03, @sobitneupane, @lschurr, @ishpaul777 eroding to Weekly issue.

@sobitneupane
Copy link
Contributor

PR in review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests