Tooltip improvements #91
Conversation
Chef-Cheems
requested review from
Chef-Chungus,
hachiojidev and
RabbitDoge
as code owners
| // then prevent closing it. | ||
| clearTimeout(hideTimeout.current); | ||
| } | ||
| if (e.target === tooltipElement) { |
There was a problem hiding this comment.
else if here is better I think. What do you think?
There was a problem hiding this comment.
It doesn't really matter actually since its impossible for both conditions to be true at the same time (since target can't be 2 things at the same time). So mostly matter of taste, I personally think its easier to read without else if 🙂
There was a problem hiding this comment.
Yes, true. I said it because of redundant check in runtime but if it is better readability I agree to stay it like this instead of little performance gain
There was a problem hiding this comment.
Hm.... that's actually a good point, thanks! 👍
But yeah, maybe better to not over-optimize stuff for now for the sake of code readability, and there are probably other pieces of code that need optimization much more than this 🙂
I'm thinking of getting some really cheap phone and laptop someday to see how it all works on low-power devices, easy to forget that most users don't have latest and greatest hardware.
There was a problem hiding this comment.
I am also a fan of avoiding else if at all cost. Easier to read, less bugs, easier to test.
| ${({ theme, hasTooltip }) => | ||
| hasTooltip && | ||
| ` | ||
| text-decoration: underline dotted ${theme.colors.textSubtle}; | ||
| text-underline-offset: 0.1em; | ||
| `} |
There was a problem hiding this comment.
This is coupling the Text component with Tooltip. I think it is better to create a component in the tooltip that uses <Text /> or do it at the project level.
There was a problem hiding this comment.
For example export a TooltipText component
There was a problem hiding this comment.
@hachiojidev hm.... how about just renaming the prop to something more generic like underlined or dotted-underlined? Seems a bit excessive to create a separate component for the same component with such little difference. 🤔
There was a problem hiding this comment.
I don't think it is excessive. This is a very specific style.
text-decoration: underline dotted ${theme.colors.textSubtle};
text-underline-offset: 0.1em;
| const tooltipHoverRef = useRef(false); | ||
| const tooltipHoverTimeoutRef = useRef<number>(); | ||
| const isHoveringOverTooltip = useRef(false); | ||
| const hideTimeout = useRef<number>(); |
There was a problem hiding this comment.
useRef<typeof setTimeout>
There was a problem hiding this comment.
yep, fixed. Can't wait when TS will start understanding if its browser or node project 🙁
There was a problem hiding this comment.
Oh shit, CI pipeline didn't like the types.... ugh...
There was a problem hiding this comment.
OK, something is different between FE repo and Toolkit TS compilation.
I'm aware of the return typeof setTimeout thing, used it myself here and you used it here (btw I just noticed you use typeof setTimeout but you actually need typeof setInterval, although it doesn't really matter I guess).
In this repo TS is not ok with that during rollup build, even though it shows no problems in VSCode.
Anyway, I chose to specifically use browser's window.setTimeout that returns plain old number instead of complicated NodeJS Timeout type that is not applicable in browser anyway. It actually makes a bit more sense to me since this code is not supposed to be run outside of browser anyway, and in browser its setTimeout returns just number.
| // then prevent closing it. | ||
| clearTimeout(hideTimeout.current); | ||
| } | ||
| if (e.target === tooltipElement) { |
There was a problem hiding this comment.
I am also a fan of avoiding else if at all cost. Easier to read, less bugs, easier to test.
| import styled from "styled-components"; | ||
| import Text from "./Text"; | ||
|
|
||
| const TooltipText = styled(Text)` | ||
| text-decoration: ${({ theme }) => `underline dotted ${theme.colors.textSubtle}`}; | ||
| text-underline-offset: 0.1em; | ||
| `; | ||
|
|
||
| export default TooltipText; |
There was a problem hiding this comment.
This is exactly what I was thinking...but exported from the Tooltip. No worries, we can think about alternative solutions later.
In this PR:
hasTooltipprop for<Text />to show dotted underlineuseTooltiparguments instead of separate parameters.