Details

Reviewers

ashoat
atul
ginsu
tomek

Commits

rCOMM7bcc0548b907: [native] Show message tooltip on press in the pinned messages screen

Summary

We need to measure vertical bounds in order to display the tooltip within the pinned messages screen. Confirming functionality of each tooltip action will come in the following diff.

Adding @ashoat since this is the approach we discussed in our sync.

Linear: https://linear.app/comm/issue/ENG-3460/show-message-tooltip-on-press-in-the-pinned-messages-screen

Depends on D7673

Test Plan

Please watch the video to show the tooltip now appearing in the screen

Diff Detail

Repository

rCOMM Comm

Branch

pinned_messages_native

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

rohan created this revision.May 5 2023, 5:55 PM

rohan edited the test plan for this revision. (Show Details)May 5 2023, 5:56 PM

rohan attached a referenced file: F524386: Simulator Screen Recording - iPhone 14 Pro - 2023-05-05 at 20.44.44.mp4. (Show Details)

Pass in messageVerticalBounds={null} from the toggle pin modal since we don't want the tooltip there

Harbormaster failed remote builds in B19090: Diff 26138!May 5 2023, 6:08 PM

Harbormaster completed remote builds in B19091: Diff 26139.May 5 2023, 6:18 PM

rohan requested review of this revision.May 5 2023, 6:18 PM

Concerned that we're not checking for height and pageY being set

native/chat/message-results-screen.react.js
39	We can just use the default `undefined` instead of specifying `null` I think
40	Can we give this a more specific name? We can just use the default `undefined` instead of specifying `null` I think
156	Elsewhere where we measure vertical bounds, we have a condition that looks like this: if ( height === null \|\| height === undefined \|\| pageY === null \|\| pageY === undefined ) { return; } Did you test this on Android?

This revision now requires changes to proceed.May 6 2023, 2:07 AM

rohan added inline comments.May 6 2023, 4:05 AM

native/chat/message-results-screen.react.js
156	I did (without this condition), and didn't experience on crashing on Android (below) or iOS (in the test plan), so that's why I didn't include it, as to me, either `messageVerticalBounds` defaults to undefined in `React.useState()`, or if `height` or `pageY` is `null` or `undefined`, it wouldn't be much different. I can add this check for consistency / in the event that I'm missing something obvious however

rohan attached a referenced file: F524681: android_tooltip.mp4. (Show Details)May 6 2023, 4:05 AM

Use the default undefined instead of specifying null in React.useState() (tested to confirm this has the exact same behavior as before).

Responded in-line to the comment about the condition prior to setting the vertical bounds & updated to include that check

Harbormaster completed remote builds in B19093: Diff 26141.May 6 2023, 4:26 AM

Let’s include the condition. Looking at Git history for MessageList, I seem to have added the condition in an unrelated Android diff in 2019 when it was still a personal project… so not much detail in the commit message, and it might reflect an outdated issue in React Native… but doesn’t hurt to check just in case, and for consistency

In D7735#229395, @ashoat wrote:

Let’s include the condition. Looking at Git history for MessageList, I seem to have added the condition in an unrelated Android diff in 2019 when it was still a personal project… so not much detail in the commit message, and it might reflect an outdated issue in React Native… but doesn’t hurt to check just in case, and for consistency

Makes sense - I've updated the diff to include the condition

Thanks @rohan!

This revision is now accepted and ready to land.May 6 2023, 9:08 AM

rohan added a child revision: D7742: [native] Show a pin icon next to pinned messages.May 8 2023, 6:52 AM

Import ScrollView from react-native-gesture-handler (fixes scroll on
Android)

Harbormaster completed remote builds in B19148: Diff 26218.May 8 2023, 12:00 PM

Rebase

Harbormaster completed remote builds in B19157: Diff 26228.May 8 2023, 12:32 PM

Closed by commit rCOMM7bcc0548b907: [native] Show message tooltip on press in the pinned messages screen (authored by RohanK6 <rohan.khanderia@gmail.com>, committed by rohan). · Explain WhyMay 8 2023, 12:38 PM

This revision was automatically updated to reflect the committed changes.

rohan added a commit: rCOMM7bcc0548b907: [native] Show message tooltip on press in the pinned messages screen.