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.
Details
Details
- Reviewers
tomek atul • abosh - Commits
- rCOMM843bcd8b4fa5: [web] Introduce `TooltipContext`
The context is not used yet, tested after other diffs in stack.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| web/chat/tooltip-provider.js | ||
|---|---|---|
| 35 ↗ | (On Diff #15849) | eslint-disbale will be implemented if next diffs |
Comment Actions
| web/chat/tooltip-provider.js | ||
|---|---|---|
| 41 ↗ | (On Diff #15849) | 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. |
Comment Actions
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 ↗ | (On Diff #15849) | 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 ↗ | (On Diff #15849) | If the only purpose of a function is to return a value, we can use a shorthand |
| 69 ↗ | (On Diff #15849) | This fragment doesn't seem to be necessary |
| web/chat/tooltip-utils.js | ||
| 18–22 ↗ | (On Diff #15849) | We should prefer readonly. Is there a really good reason for not having it? |
Comment Actions
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 ↗ | (On Diff #15849) | 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 ↗ | (On Diff #15849) | In what scenarios is the return value of renderTooltip null/undefined? (Guessing this will become clear after I get through the later diffs) |
| 36 ↗ | (On Diff #15849) | 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? |
Comment Actions
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 ↗ | (On Diff #15849) | 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). |
| 19 ↗ | (On Diff #15849) | It's undefined only when we provide default function value in line 25 below. |
| 36 ↗ | (On Diff #15849) | 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 ↗ | (On Diff #15849) | True, I should have removed all the functions except from those provided by context type instead of just removing the implementation. Sorry for that. |
| 69 ↗ | (On Diff #15849) | It was left accidentally. Thanks for catching |
| web/chat/tooltip-utils.js | ||
| 18–22 ↗ | (On Diff #15849) | No, sorry for missing it |