Details

Reviewers

tomek
atul
• abosh

Commits

rCOMM843bcd8b4fa5: [web] Introduce `TooltipContext`

Summary

Introduce context for rendering tooltip. It is similar to the one used with Menu component. It provides functions for rendering and clearing tooltip.
The funcitons will be implemented if next diffs.

Test Plan

The context is not used yet, tested after other diffs in stack.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• jacek created this revision.Aug 23 2022, 4:41 AM

• jacek held this revision as a draft.

Herald added a subscriber: ashoat. · View Herald TranscriptAug 23 2022, 4:41 AM

Harbormaster completed remote builds in B11526: Diff 15849.Aug 23 2022, 4:51 AM

• jacek added a child revision: D4909: [web] Introduce `clearTooltip` and `renderTooltip` functions in `TooltipContext`.Aug 23 2022, 5:25 AM

• jacek added inline comments.Aug 23 2022, 5:36 AM

web/chat/tooltip-provider.js
35	`eslint-disbale` will be implemented if next diffs

• jacek published this revision for review.Aug 23 2022, 8:06 AM

Makes sense, but adding @tomek and @atul as blocking so they can look. feel free to change blocking status.

web/chat/tooltip-provider.js
41	Nit, but if there's no reason for this to be `null` vs `undefined`, you can omit the `null` argument. I believe the same applies for `useRef` as well. But if you like it being explicitly `null`, then I think you should leave it as is. Read more

For the majority of our contexts we split them into two files: one with context declaration and one with the provider (e.g. keyboard-state or sqlite-context). We don't have a naming convention for these, but probably tooltip-context and tooltip-context-provider would make a lot of sense.

web/chat/tooltip-provider.js
36–57	In terms of diff stack structure, this isn't ideal. It makes sense to introduce an empty context with just empty functions, but it is really confusing for a reviewer to see things like `timer`, `onMouseLeave`, etc. here, without any explanation.
61–66	If the only purpose of a function is to return a value, we can use a shorthand
69	This fragment doesn't seem to be necessary
web/chat/tooltip-utils.js
18–22	We should prefer readonly. Is there a really good reason for not having it?

This revision now requires changes to proceed.Aug 25 2022, 2:50 AM

Couple of questions inline which I'm guessing will be answered once I take a look at subsequent diffs in the stack.

Accepting since @tomek already covered all of the changes I think need to be made before this diff is ready to be landed.

web/chat/tooltip-provider.js
15	Isn't the `clearTooltip()` in the object returned by `renderTooltip(...)` redundant given that `TooltipContext` also has a `clearTooltip()` function? Let me know if I'm missing something here
19	In what scenarios is the return value of `renderTooltip` `null/undefined`? (Guessing this will become clear after I get through the later diffs)
36	It probably becomes evident in later diffs, but could you explain why we're using `symbol`? My understanding that `Symbol` is a new ES6 primitive that enables non-string property names... it's not immediately obvious to me why we need to hold onto a `tooltipSymbol`, could you give some quick context?

For the majority of our contexts we split them into two files: one with context declaration and one with the provider (e.g. keyboard-state or sqlite-context). We don't have a naming convention for these, but probably tooltip-context and tooltip-context-provider would make a lot of sense.

We didn't use this convention for web app. In web app, both menu and modal context were defined together with the provider, so I followed the convention used there. I'm not sure if it's better to keep the context type separated from the provider implementation, but I can separate it if you like.

web/chat/tooltip-provider.js
15	No, the API provides `clearTooltip` function that clears any currently displayed tooltip (Until now, the message list component did it when scrolling a list of messages). The `clearTooltip` returned from `renderTooltip()` is used to clear only the one passed to `renderTooltip`.
19	It's `undefined` only when we provide default function value in line 25 below.
36	`Symbol` here has a similar function as in `menuContext` - it is used as a unique ID to identify the currently rendered tooltip. Such identification is necessary to correctly handling mouse events, as we want to ignore the events from tooltips that have been displayed in the past.
36–57	True, I should have removed all the functions except from those provided by context type instead of just removing the implementation. Sorry for that. I'll leave it in this commit as it is now if it's not a big problem.
69	It was left accidentally. Thanks for catching
web/chat/tooltip-utils.js
18–22	No, sorry for missing it