Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New tooltip + making the DropdownTrigger be a Button #276

Merged
merged 3 commits into from
Jul 30, 2020

Conversation

gnapse
Copy link
Contributor

@gnapse gnapse commented Jul 24, 2020

Short description

This PR introduces a new tooltip, and it makes small but breaking changes to the dropdown trigger and the popover.

There will be an accompanying PR in Twist to adopt all these changes. It is in the works.

About the new tooltip

The new tooltip is internally implemented using reakit's Tooltip. It has less customization options. Most of what was dropped has a reason:

  • delays: not recommended for tooltips (these were brought back during this PR but they are no longer customizable, which is IMO a good thing though we may revisit that in the future)
  • vague positioning: I'm not sure what this used to do, but assuming it is that the tooltip will take into account not clashing with viewport edges or something like that, that's now the default and it is not recommended to force it into one direction.
  • arrows: reakit supports it and I did not want to drop support for this even if our design system seems to be driving away from using arrows. But I devoted more time than I could afford making it work and couldn't.

The new tooltip introduces another restriction over the previous one: it has to be wrapping a single element, not many elements. This makes sense anyway, as it is on focus and hover when it shows up.

About the changes to popover and dropdown trigger

These are introducing the same restrictions on the trigger to be a single element. We no longer can set a dropdown or popover trigger to be various elements.

In addition to that, the Dropdown.Trigger is now a Button and it supports receiving a tooltip="..." prop directly. I now realize I could've enabled it to receive some of the other customization options of the Button (variant, size) but I can do that later. Probably it won't be needed as we'll have a new dropdown menu replacing most of its uses.

PR Checklist

  • Added tests for bugs / new features
  • Updated docs (storybooks, readme)
  • Executed npm run validate and made sure no errors / warnings were shown
  • Described changes in CHANGELOG.md
  • Bumped version in package.json
  • Updated all static build artifacts (npm run build-all)

Versioning

These are significan breaking changes and will require a new major release. I may coordinate it to include another major change that is coming (the new and accessible dropdown menu component).

@gnapse gnapse added Type:Feature dependencies Pull requests that update a dependency file Component:Tooltip Versioning:BREAKING-CHANGE PRs that would result in a major version release labels Jul 24, 2020
@gnapse gnapse self-assigned this Jul 24, 2020
@henningmu
Copy link
Contributor

vague positioning

Yup, it prevented clashing with the sides or rendering part of the tooltip offscreen. Looks like you're correct and that's the new default 👍

In testing I found that the tooltips in the colorpickers are rendered below the dropdown body:

Screenshot 2020-07-27 at 14 19 48

Apart from that everything looks good 🙌

Copy link
Contributor

@henningmu henningmu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested it again and it works nicely everywhere now.

One thing regarding this though:

delays: not recommended for tooltips

Looking at the recent discussions in Twist about tooltips it seems like we need some delay. Especially, for the macOS app ref. Do you think we can solve that with a delayed css animation? Or should this component have real delay support?

I don't think this is blocking for this PR though, we could add it as a feature later on 👍

(And sorry for not bringing it up earlier).

@gnapse
Copy link
Contributor Author

gnapse commented Jul 30, 2020

I'll explore the animation thing and see if it allows us to introduce a certain small delay. Though the tooltip accessibility recommendations do not say anything about delaying things, but they also are not explicit about immediacy (ref).

@gnapse
Copy link
Contributor Author

gnapse commented Jul 30, 2020

Achieved the tooltip delay (reakit is so sweet in giving us so much power in customizing it), and I agree that a very small delay is beneficial. Especially the fact that without it if you pass the mouse cursor over an element with a tooltip even very rapidly, the tooltip flashes on and off, while what every one would want is to not appear in that case. reach-ui got that one right, but it was harder to customize the delay they have (too long when you leave the element) than in reakit, where it was relatively easy without any patching.

Thanks for pushing for this. Much better now. I still left it as not something we allow to customize for the moment but we can revisit and discuss that decision.

@gnapse gnapse merged commit b84c7f8 into dev Jul 30, 2020
@gnapse gnapse deleted the feature/tooltip-and-related-stuff branch July 30, 2020 17:18
Copy link
Contributor

@vishnugopal vishnugopal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component:Tooltip dependencies Pull requests that update a dependency file Type:Feature Versioning:BREAKING-CHANGE PRs that would result in a major version release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants