Page MenuHomePhabricator

[web] Use new tooltip in composed message
ClosedPublic

Authored by jacek on Aug 23 2022, 5:10 AM.
Tags
None
Referenced Files
F3367983: D4920.id15861.diff
Mon, Nov 25, 5:39 PM
Unknown Object (File)
Fri, Nov 22, 6:52 AM
Unknown Object (File)
Mon, Nov 11, 1:52 AM
Unknown Object (File)
Mon, Oct 28, 5:06 AM
Unknown Object (File)
Mon, Oct 28, 5:06 AM
Unknown Object (File)
Mon, Oct 28, 5:06 AM
Unknown Object (File)
Mon, Oct 28, 5:04 AM
Unknown Object (File)
Oct 24 2024, 5:31 PM
Subscribers

Details

Summary

Use new tooltip API in composed message.

Test Plan

Flow; Tested with the rest of diffs stack.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

jacek held this revision as a draft.
tomek requested changes to this revision.Aug 26 2022, 9:25 AM
tomek added inline comments.
web/chat/composed-message.react.js
204–205 ↗(On Diff #15861)

I guess you could simplify it by using a spread

206–210 ↗(On Diff #15861)

It would be more performant to memoize separately all the things that don't depend on event, e.g. tooltipLabels and tooltipSize change only when action or messageTimestamp change, which means that they can be reused for a couple of different onMouseEnter calls.

214–215 ↗(On Diff #15861)

preventDisplayingBelowSource doesn't seem really needed because we can just use availableTooltipPositions without BOTTOM. This will simplify the api.

224 ↗(On Diff #15861)

Use shorthand

234 ↗(On Diff #15861)

Can we use early exit for this? It is wasteful to compute all the non-hook things if we're not going to use them

237 ↗(On Diff #15861)

It is a little confusing to see style assigned to a position

This revision now requires changes to proceed.Aug 26 2022, 9:25 AM
web/chat/composed-message.react.js
204–205 ↗(On Diff #15861)

There is some flow issue, when I try to do it - getBoundingClientRect() in Flow is defined to return ClientRect instead of DomRect from specification what makes impossible to remove x and y from the object:

declare class HTMLElement extends Element {
...
  getBoundingClientRect(): ClientRect;
...
214–215 ↗(On Diff #15861)

No, when we display tooltip in LEFT, LEFT_TOP, RIGHT, RIGHT_TOP position it may be higher that the message, what will cause the tooltip to cover the inline sidebar.
The only positions that guarantees that it will not exceed the bottom edge of the message are TOP, and LEFT_BOTTOM, RIGHT_BOTTOM (because the tooltip is aligned to the bottom so it "grows" up) and we don't want to be limited to these positions when we just want not to cover the inline sidebar

address feedback - moved logic into common hook

This revision is now accepted and ready to land.Sep 2 2022, 3:27 AM