Implementation of functions provided by tooltip context. The render function returns callback that should be called later by component calling to render tooltip (to correctly hide tooltip with delay).
Details
Details
Flow; The functions has been tested together with rest of diffs stack.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
| web/chat/tooltip-provider.js | ||
|---|---|---|
| 8 | I haven't seen the units for the variable in the variable name in the codebase (I just searched for "Timeout"), but I did see a pattern with naming timeouts in milliseconds:   I think the Ms can probably be dropped since this variable is never changed once it's initialized, so the reader can just read the comment and know both its value and units. | |
| 61–65 | I have a question, is this a helper function or is it necessary for calling clearTooltip since we're using context? That is, suppose somewhere down the component tree, some component wants to clear the tooltip. Would they be unable to call clearTooltip since they don't have access to tooltipSymbol.current which needs to be a parameter (since tooltipSymbol.current is part of the current tooltip context)? And is that why we have to create clearCurrentTooltip, so they can call a function without worrying about not having access to the parameters? Or is clearCurrentTooltip just a helper function? | |
| 72 | In RenderTooltipParams, neither newNode nor tooltipPosition can be null or undefined (no question mark before the type), so when would !newNode || !newTooltipPosition be true? Maybe I am missing something because of unfamiliarity with Flow. | |
| 77–78 | Can probably merge these two lines and refer to newNodeSymbol as tooltipSymbol.current. At least that makes sense to me since in clearTooltip (which is the function you use to reference newNodeSymbol in the rest of this method) the first line makes sure tooltipToClose === tooltipSymbol.current. So if you remove newNodeSymbol and just refer to it as tooltip.current it makes sense that you're passing tooltip.current as the parameter to clearTooltip. That is, lines 87 and 92 can be amended to say clearTooltip(tooltip.current). | |
| web/chat/tooltip-provider.js | ||
|---|---|---|
| 8 | That's true that we don't have units in names on web, but I'm not sure if this is a convention and if it should be followed. Declaration is one place where a unit matters, but the usage is even more important. The fact that a comment suffice for declaration, doesn't mean it would be too helpful when reading the code. Having a unit in the name would make reading its usage simpler, by not having to check its declaration. The fact that the value doesn't change helps, but not by much, because it just reduces the number of usages. Ordering the approaches by my personal preference (from the worst):
So regarding this task, I think we can follow the approach we have, so skip the unit, but I would prefer to keep it (up to you, we can also update all the timeouts to have a unit. Additional thing to consider is that all the timeouts are used only in setTimeout where a meaning is always the same.). For the future we can consider introducing a better solution, but that would depend on cost vs benefit. And currently we don't have too many unit-related bugs, so it's probably not that important. | |
| web/chat/tooltip-provider.js | ||
|---|---|---|
| 8 | I agree with all you said. I would also prefer to keep the Ms in the title, but made my comment since the current pattern in the codebase is to leave a comment. I have a question about JavaScript in general, do we use SCREAMING_CASE when naming constants like this that shouldn't change and represent fixed primitive values? Because I haven't really seen that when we write JavaScript, but it could help? | |
| web/chat/tooltip-provider.js | ||
|---|---|---|
| 71 | Should this be ?RenderTooltipResult given the following on line 21: +renderTooltip: (params: RenderTooltipParams) => ?RenderTooltipResult, ? | |
| 72 | In what scenario do we anticipate this conditional being true? It's a long stack so maybe it becomes relevant later? | |
| 73 | If the return type of renderTooltip is ?RenderTooltipResult can we just return null or undefined here instead of empty functions (unless they're filled out in later diffs I guess)? | |
| 77 | If I understand correctly we're basically using Symbol() to uniquely identify each tooltipNode that gets passed through renderTooltip(...)? | |
| 85–91 | We could maybe pull the contents of onMouseLeaveCallback() outside of the return block to reduce indentation and clean things up a bit? Don't feel strongly, up to you | |
| web/chat/tooltip-provider.js | ||
|---|---|---|
| 8 | Currently we're not using it, but it might be a good idea to start doing so. In this case the consistency matters, so if we want to use SCREAMING_CASE it would be a good idea to change all the places at the same time. We also need to make sure that they are actually constants so it is not possible to mutate them in any way. A thing to consider is how many such constants do we have. I don't think the readability would be great if a lot of code will be written using uppercase letters. And the last thing is if the fact that something is a constant should matter that much to other parts of the code? Sometimes yes, because we can assume that it isn't going to change, but in most of the places it doesn't matter - we're just using the current value. | |
| web/chat/tooltip-provider.js | ||
|---|---|---|
| 61–65 | The clearCurrentTooltip is a function provided by the context, that allows any component to clear any currently displayed tooltip. The clearTooltip is a callback used to clear only the tooltip with symbol from the function parameter (so it's passed to components to be executed in onMouseLeaveCallback()) | |
| 71 | I think it doesn't have to, as this callback never returns null. Flow doesn't prevent assigning this function with wider type to context. | |
| 72 | Probably in no scenario - I'll remove this check | |
| 77 | Exactly | |
| 77–78 | I think current code is for me easier to read - as we know, that we refer to the new symbol when we use it. The suggested change makes problems with flow because tooltipSymbol.current can be null. So, I think, it's better to leave it as it is. | |