Page MenuHomePhabricator

[web] Update a tooltip when it changes
ClosedPublic

Authored by tomek on Dec 29 2022, 2:05 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 23, 4:35 AM
Unknown Object (File)
Sat, Nov 23, 3:59 AM
Unknown Object (File)
Mon, Nov 11, 2:46 AM
Unknown Object (File)
Sun, Nov 10, 6:26 AM
Unknown Object (File)
Sat, Nov 9, 12:02 PM
Unknown Object (File)
Sat, Nov 9, 12:02 PM
Unknown Object (File)
Wed, Nov 6, 1:58 AM
Unknown Object (File)
Tue, Nov 5, 2:08 AM
Subscribers

Details

Summary

Every time a tooltip is changed, render an updated version using the api introduced in this stack.

Depends on D6087

Test Plan

Tested a couple of scenarios including copying a message and then e.g. keeping the cursor on a tooltip, exiting a tooltip, entering a different message, changing threads, etc. Also tested clicking a copy button multiple times.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

web/utils/tooltip-utils.js
566–567 ↗(On Diff #20298)

Haven't found an easy way to unset these when we no longer need them. We could consider introducing on tooltip close callback and pass it to render tooltip, but I tried that previously and making it work safely might be intensive. The fact that we keep these set shouldn't affect the performance significantly.

577 ↗(On Diff #20298)

Saving this on mouse enter because this is the last thing independent of tooltip content. Other variables depend on it so after e.g. tooltip actions are changed, they are also likely to change.

630 ↗(On Diff #20298)

Flow thinks this is nullable here

tomek requested review of this revision.Dec 29 2022, 2:20 AM
web/utils/tooltip-utils.js
613 ↗(On Diff #20298)

Why do we need this? I'm not sure if I understand, but it looks like we recreate entire tooltip when internal state of one of the buttons changes. Shouldn't it happen automatically after successful state changes in useMessageCopyAction

web/utils/tooltip-utils.js
613 ↗(On Diff #20298)

I tried to explain this earlier in the stack in the summary of D6075. But basically, the flow is that we create tooltips based on props / state but then send them to the provider only on mouse enter. So even if the hooks rerender, it won't affect what the provider is rendering, because it received and saved a node that was rendered earlier. The mentioned diff contains a discussion about possible solution and calling update seems to be the best one so far.

przemek added inline comments.
web/utils/tooltip-utils.js
613 ↗(On Diff #20298)

Thanks for clarifying

This revision is now accepted and ready to land.Dec 30 2022, 3:17 AM
This revision was automatically updated to reflect the committed changes.