Page MenuHomePhabricator

[native] Clean up Tooltip component
ClosedPublic

Authored by ashoat on Feb 3 2023, 12:07 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Oct 22, 3:19 AM
Unknown Object (File)
Tue, Oct 22, 3:19 AM
Unknown Object (File)
Tue, Oct 22, 3:19 AM
Unknown Object (File)
Tue, Oct 22, 3:19 AM
Unknown Object (File)
Tue, Oct 22, 3:19 AM
Unknown Object (File)
Tue, Oct 22, 3:03 AM
Unknown Object (File)
Mon, Oct 21, 3:54 PM
Unknown Object (File)
Oct 2 2024, 11:35 AM
Subscribers

Details

Summary

Lots of technical debt accumulated over the last couple months here. Big part of the problem is that we have to "drill" anything we need in from a hook.

This diff replaces the static spec we have today with a React component, which allows callers to use hooks to fetch any necessary data. It also looks pretty clean.

It also addresses a bug resulting from the relationship between options and icons not be considered - it was possible for icons to be matched to incorrect options.

After this diff, we should have no message-related code inside Tooltip, and going forward we should make sure we don't regress on that.

TooltipContext handles figuring out how many items can fit, as well as the relationship with the action sheet.

This is a pretty large diff, but I opted not to break it up because it otherwise would've been hard to see the changes to the Tooltip component if I had to keep both the old and the new ones alive across multiple diffs.

Test Plan

Played around with all of the tooltips in the iOS simulator. I need to test Android as well. @ginsu, could use some help here

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

After this diff, we should have no message-related code inside Tooltip, and going forward we should make sure we don't regress on that.

Scratch that, showEmojiKeyboard is still here. @ginsu, can you figure out a way to clean that up?

Created ENG-2922 as a follow-up. Can't prioritize right now but would be good to get done at some point

ginsu requested changes to this revision.Feb 3 2023, 1:13 PM

Did some more tests with the tooltips and found some regressions that should be addressed:

  1. shouldShowMore seems like it is not working as expected. I modified text-message-tooltip-modal to have more than four TooltipItem; however, instead of the more TooltipItem being rendered, I would just get all five TooltipItem

Screenshot 2023-02-03 at 3.49.58 PM.png (1×1 px, 651 KB)

  1. The tooltip is not going away after pressing a TooltipItem. I attached some demo videos to show better what I am talking about

Before:

After:

Scratch that, showEmojiKeyboard is still here. @ginsu, can you figure out a way to clean that up?

I can try and get this done by the Monday dev sync

native/tooltip/tooltip-item.react.js
39 ↗(On Diff #21930)

Should this be memoized? I am assuming that since renderIcon uses the useCallback hook in the respective tooltip modal components (text-message-tooltip-modal, multimedia-message-tooltip-modal, etc.) memorizing is unnecessary here?

This revision now requires changes to proceed.Feb 3 2023, 1:13 PM

@ginsu's first regression was a result of using the same ID for multiple TooltipItems. I added some code that prints a warning when the dev does that

native/tooltip/tooltip-item.react.js
39 ↗(On Diff #21930)

Memoizing this would have no effect since it's immediately wrapped by a new array by nature of being in a list with the SingleLine on line 44. To properly memoize this we would have to memoize that as a React.Fragment, but if we memoizing every children prop our JSX would quickly get unreadable. I think it's fine not to worry about stuff like that

native/tooltip/tooltip.react.js
559–566 ↗(On Diff #21930)

I initially forgot to port this logic over, which is what @ginsu's second video shows. Fixed it by passing a closeTooltip prop to TooltipItem, but as a result the TooltipItem component we pass to TooltipMenu now depends on state, and has to be generated inside of a component instead of being defined at the top-level. To take advantage of React.useMemo I moved the definition of getTooltipItem ConnectedTooltip

This revision is now accepted and ready to land.Feb 5 2023, 12:26 AM
This revision was automatically updated to reflect the committed changes.