web/chat/tooltip-provider.js
139–148	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	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	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

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	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	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	For me, it looks like the line have 80 characters
139–148	Ok, that makes sense
web/chat/tooltip.css
50–60	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	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

This revision was automatically updated to reflect the committed changes.