Page MenuHomePhabricator

[web] Introduce tooltip container with positioning in `TooltipContext`
ClosedPublic

Authored by jacek on Aug 23 2022, 4:50 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Dec 7, 7:28 PM
Unknown Object (File)
Sun, Dec 1, 7:30 PM
Unknown Object (File)
Sun, Dec 1, 7:30 PM
Unknown Object (File)
Sun, Dec 1, 7:30 PM
Unknown Object (File)
Sun, Dec 1, 11:43 AM
Unknown Object (File)
Thu, Nov 28, 2:56 PM
Unknown Object (File)
Mon, Nov 25, 11:58 PM
Unknown Object (File)
Mon, Nov 25, 3:21 PM
Subscribers

Details

Summary

Introduce tooltip container that displays tooltip in correct position depending on provided tooltip style object

Test Plan

Flow; Tested with the rest of diffs stack.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

jacek held this revision as a draft.
tomek requested changes to this revision.Aug 25 2022, 10:49 AM
tomek added inline comments.
web/chat/tooltip-provider.js
139–148 ↗(On Diff #15852)

Usually we need a good reason to use inline style property. For tooltipContainerStyle it makes sense because we determine properties based on value of js variables. In this case, though, it would probably make sense to have a couple of classes in css and conditionally decide which to use - just like it is done for tooltipClassName.

160 ↗(On Diff #15852)

Can we wrap it with a callback? I know that we're inside a memo, but there's no good reason for recreating this on every memo update.

web/chat/tooltip.css
50–60 ↗(On Diff #15852)

Is there a way to avoid child combinators? I think they're bad for maintainability because it is really hard to find all the styles that affect a particular element. If we directly use classes, this problem disappears.

This revision now requires changes to proceed.Aug 25 2022, 10:49 AM
atul accepted this revision.EditedAug 25 2022, 8:55 PM

Accepting since @tomek's feedback covers the changes I think need to be made before this diff is ready to land.

web/chat/tooltip-provider.js
137 ↗(On Diff #15852)

Off-topic: Hm weird looks like prettier goes with 81 characters here even though it seems like it'd be easy to break apart into separate lines eg

const {
  verticalPosition,
  horizontalPosition,
  alignment,
} = tooltipPosition;
139–148 ↗(On Diff #15852)

Agree, since we only have four cases I think it would be cleaner to just break them out in the CSS and pick the class names here

web/chat/tooltip-provider.js
137 ↗(On Diff #15852)

For me, it looks like the line have 80 characters

139–148 ↗(On Diff #15852)

Ok, that makes sense

web/chat/tooltip.css
50–60 ↗(On Diff #15852)

The reason of the solution here is to change the alignment inside the tooltipNode depending on its container position.
Maybe the solution here would be to include these styles directly in tooltip, but in such case we'd need to pass an additional property to the tooltip component before executing renderTooltip.

changed JS styles to CSS classes

tomek added inline comments.
web/chat/tooltip.css
50–60 ↗(On Diff #15852)

I'm always skeptical about child / descendant combinators and usually prefer a solution where it is not needed. Adding a new prop might be worth considering, because the fact that the content of a tooltip will align differently shouldn't be a concern of the provider. Especially, if we decide to render a different tooltip it might be incorrect to set the alignment. The tooltip content should be informed about its position and should react accordingly.

This revision is now accepted and ready to land.Aug 30 2022, 6:21 AM

remove tooltip align styles