Page MenuHomePhabricator

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

Authored by rohan on May 5 2023, 5:55 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Nov 10, 2:54 PM
Unknown Object (File)
Mon, Nov 4, 4:14 PM
Unknown Object (File)
Mon, Oct 28, 6:39 AM
Unknown Object (File)
Oct 2 2024, 6:04 AM
Unknown Object (File)
Oct 2 2024, 12:07 AM
Unknown Object (File)
Sep 28 2024, 8:27 AM
Unknown Object (File)
Sep 28 2024, 8:27 AM
Unknown Object (File)
Sep 28 2024, 8:27 AM
Subscribers

Details

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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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

rohan requested review of this revision.May 5 2023, 6:18 PM
ashoat requested changes to this revision.May 6 2023, 2:07 AM

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

native/chat/message-results-screen.react.js
39 ↗(On Diff #26139)

We can just use the default undefined instead of specifying null I think

40 ↗(On Diff #26139)
  1. Can we give this a more specific name?
  2. We can just use the default undefined instead of specifying null I think
156 ↗(On Diff #26139)

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
native/chat/message-results-screen.react.js
156 ↗(On Diff #26139)

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

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

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

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

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

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