BREAKING: Re-implemented Tooltip component
Short description
This PR re-implements the tooltip using @reach/tooltip instead of Reakit's tooltip. More on the reasons for this can be found below under "Long description".
This also changes the tooltip props interface a bit. In ways that do not affect us much or at all. Most notably, here are the relevant changes:
-
The
positionprop now supports only four values:top,right,bottom,left. This is mainly due to the fact that Reakit used to provide positioning calculation internally, whereas Reach-UI's toolkit leaves that to you, so I had to implement the math that computes positioning for each of these four cases (based on Todoist's Reach-UI-based tooltip). But we rarely ever change this. We use mostlytop(the default), and in a couple of cases we manually selectbottomandright. -
The
gapSizeprop was renamed tooffset. This is optional and I can revert it. Butoffsetsounds more natural, and we're having breaking changes anyway. -
Tooltipno longer accepts aclassName. It was replaced by theexceptionallySetClassNameprop instead. -
I also took the liberty to remove this feature introduced not long ago, that I was challenging in that PR. This would also be a breaking change, but it is pending to see the discussion about this in that PR.
Update: this was accepted, with an added change, to allow customizing icon-only tooltips by enabling consumers to suppress the implicitly generated tooltip. See https://github.com/Doist/reactist/pull/636#issuecomment-1099075813.
Long description
The main reason to do the library provider change is that I finally found out the reason for this issue. TL;DR about this issue: we cannot do this today:
<Tooltip content="tooltip text"> // <-- This is key to the problem
<ButtonLink
variant="primary"
as={Link} // <-- and this is key to the problem too
to="/somewhere"
>
Click me
</ButtonLink>
</Tooltip>
We cannot combine a button wrapped by a tooltip that at the same time wants to render as={SomethingElse}. Hence that polymorphic link workaround. Today I finally confirmed that the Reakit way of doing tooltips is to blame. I think it's due to this TooltipReference approach they follow. Compare our code with the Reakit tooltip vs. the code for the Reach-UI tooltip:
| Reakit | Reach-UI |
|---|---|
|
|
See how Reach-UI manages to have a single call to React.cloneElement and that's it? Whereas Reakit's approach includes this thing wrapping the call to React.cloneElement.
PR Checklist
- [x] Added tests for bugs / new features
- [ ] Updated docs (storybooks, readme)
- [x] Executed
npm run validateand made sure no errors / warnings were shown - [x] Described changes in
CHANGELOG.md - [ ] Bumped version in
package.jsonandpackage-lock.json(npm --no-git-tag-version version <major|minor|patch>) ref - [ ] Updated all static build artifacts (
npm run build-all)
Versioning
Breaking change. Implies that our next release will increase our major version number.
During QA I discovered that after clicking the button, the tooltip will no longer render until the page has been refreshed. Do you know why that may be?
Nice catch. It's an error in how I implemented here the fix to prevent showing the tooltip under certain circumstances. It turns out is not that simple, and this is more easily handled when using the Reakit tooltip.
Which led me to an idea about how to fix this while keeping the Reakit tooltip. However, I'm running into very weird issues with running twist linked to reactist locally (which is key for me to try what I'm thinking). This has turn out to be a huge time sink, so I may leave this for later during the week. Let's put it on hold for now.
This is no longer relevant, as it moves us in the opposite direction. That tooltip has since been changed to use Ariakit (the successor of Reakit) instead of Reakit.
I'm not sure if the tooltip with Ariakit solved the problem that I was attempting to solve by moving to Reach-UI, but if it does not, we'll have to figure it out within Ariakit, as there are bigger problems in sticking with Reach-UI, and we've moved away from it entirely.