Introduce tooltip container that displays tooltip in correct position depending on provided tooltip style object
Details
Details
Flow; Tested with the rest of diffs stack.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| 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. |
Comment Actions
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. |
| 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. |