Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
2.99k stars 2.5k forks source link

[Needs Repro] [$250] Adding an emoji doesn’t reset the scroll view, emoji is hidden #41650

Closed m-natarajan closed 1 week ago

m-natarajan commented 2 weeks 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: 1.4.70-2 Reproducible in staging?: Yes Reproducible in production?: Yes If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Expensify/Expensify Issue URL: Issue reported by: @JmillsExpensify Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1714757774028299

Action Performed:

  1. Navigate to any chat message that doesn’t have an emoji
  2. Open a new thread on that message
  3. Go back to the parent message
  4. Add an emoji

Expected Result:

When you add an emoji, the scroll position resets ever-so-slightly, so that you can see the emoji

Actual Result:

The emoji is hidden even though correctly added, because it’s below the current scroll view

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

https://github.com/Expensify/App/assets/38435837/b740eb1d-80db-4592-8af5-0830eeb5271b

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~015963598f7457d1e9
  • Upwork Job ID: 1787989439920746496
  • Last Price Increase: 2024-05-07
melvin-bot[bot] commented 2 weeks ago

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[bot] commented 1 week ago

Job added to Upwork: https://www.upwork.com/jobs/~015963598f7457d1e9

melvin-bot[bot] commented 1 week ago

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

strepanier03 commented 1 week ago

I'm unable to repro this, my chat view does scroll to show the emoji automatically. I am testing on Mac/Chrome, Android/Chrome and Android/Native.

MelvinBot commented 1 week ago

This has been labelled "Needs Reproduction". Follow the steps here: https://stackoverflowteams.com/c/expensify/questions/16989

Testing from Mac/Chrome below. https://github.com/Expensify/App/assets/10925636/7c6346f2-6697-49a4-8c9f-5a763fa4c105

dominictb commented 1 week ago

@strepanier03 The OP video is incorrect, the emoji here is the reaction to the report action item, not emoji to be sent as a message. Please try the below reproduction video:

https://github.com/Expensify/App/assets/165644294/46aae98a-b249-4f02-9c07-10244967ac8a

The steps in the OP is correct, just note to use a message that's like the 10th message in the chat (not the 1st, 2nd), it will be easier to reproduce.

@strepanier03 Let's reopen this issue.

dominictb commented 1 week ago

Proposal

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

The emoji is hidden even though correctly added, because it’s below the current scroll view

What is the root cause of that problem?

When the user goes back to the parent message, it will be the same as doing comment linking on the parent message. When we do comment linking, the linked comment will be at the bottom of the screen.

When the user adds a reaction, the ReportActionItemEmojiReactions component will show up, but it will be hidden below the screen. As of now we don't have any logic to readjust the report action item if its height expand and the reactions are hidden like this.

This is troublesome for the user because there's no visual feedback after they reacts to the comment, looks like the feature is broken.

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

If the height of a report action item changes, and the new part is being hidden, we should automatically scroll to that report action item to reveal the new part (reactions). This will ensure a smooth UX for the user

  1. In https://github.com/Expensify/App/blob/974f6b58e0acdebd1f97e8c712df0c7a267b9470/src/pages/home/report/ReportActionItem.tsx#L293 Define a containerHeightRef to store the report action item container height
    const containerHeightRef = useRef();
  2. In the container here, add onLayout callback, in this onLayout, if we detect that the height of the report action item container has changed, and the bottom of the container is hidden, but the top of the container is not, that means the report action item is the last one and the new added reactions are being hidden from user's view. In this case we should scroll to that item to make sure the reactions show.

Pseudo code:

<View style={highlightedBackgroundColorIfNeeded} onLayout={(event) => {
    if (!containerHeightRef.current || containerHeightRef.current !== event.nativeEvent.layout.height) {
        if (containerHeightRef.current && [top of the item is in view] && [bottom of the item is out of view]) {
            // If the height has changed and the new reactions added are being hidden, we need to update the scroll position
            reportScrollManager.scrollToIndex(index);
        }

        containerHeightRef.current = event.nativeEvent.layout.height;
    }
}}>
  1. For the [top of the item is in view] and [bottom of the item is out of view], we can easily check by using the scroll offset of item, the height of the item and the scroll position of the list. Or we can use something like document.elementFromPoint here to check if the top/bottom is hidden

What alternative solutions did you explore? (Optional)

NA

dominictb commented 1 week ago

@strepanier03 Friendly bump to reopen this issue since it's reproducible

dominictb commented 4 days ago

@strepanier03 Friendly bump on the above!