reactist icon indicating copy to clipboard operation
reactist copied to clipboard

BREAKING: Re-implemented Tooltip component

Open gnapse opened this issue 3 years ago • 1 comments

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 position prop 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 mostly top (the default), and in a couple of cases we manually select bottom and right.

  • The gapSize prop was renamed to offset. This is optional and I can revert it. But offset sounds more natural, and we're having breaking changes anyway.

  • Tooltip no longer accepts a className. It was replaced by the exceptionallySetClassName prop 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:

ReakitReach-UI
image image

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 validate and made sure no errors / warnings were shown
  • [x] Described changes in CHANGELOG.md
  • [ ] Bumped version in package.json and package-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.

gnapse avatar Apr 14 '22 01:04 gnapse

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.

gnapse avatar Apr 19 '22 20:04 gnapse

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.

gnapse avatar Sep 01 '22 12:09 gnapse