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

Using FloatingUI with Tooltips #1311

Merged
merged 60 commits into from Jun 30, 2023
Merged

Using FloatingUI with Tooltips #1311

merged 60 commits into from Jun 30, 2023

Conversation

gretanausedaite
Copy link
Contributor

@gretanausedaite gretanausedaite commented May 29, 2023

Changes

Added FloatingUI and used it in Tooltip:

  • Updated Tooltip to use floatingUI.
  • Tooltip uses portal
  • Updated reference functionality to use callback and set ref to user provided element
  • Added pointer events to the Tooltip to allow hovering and selection

Testing

  • Unit tests
  • Visual tests

Docs

@gretanausedaite gretanausedaite self-assigned this May 29, 2023
@gretanausedaite gretanausedaite added this to the React 3.0 milestone May 29, 2023
@gretanausedaite gretanausedaite linked an issue May 29, 2023 that may be closed by this pull request
@gretanausedaite
Copy link
Contributor Author

@mayank99 Checkpoint if the approach is correct:
Added useTooltip hook and NewTooltip React component that mimics almost all functionality previous tooltip had. I think we could make useTooltip hook to be more generic and add more options to it for customization (Tooltip vs Dialog vs Menu).
I think it's ok if we don't have same functionality as previously on Tooltips, eg. trigger prop.

@mayank99
Copy link
Contributor

I think we could make useTooltip hook to be more generic and add more options to it for customization (Tooltip vs Dialog vs Menu).

On the contrary, I think this hook should be moved inside the Tooltip file. Tooltip is very different from Menu, so it would be better not to share code.


In general looks ok so far, other than three comments.

  1. is it possible to avoid the extra div here? i am going to add an extra div in add portal container to context #1300 so it would be nice if we could just use that instead.

  2. we will definitely need a bunch of customization options. three important ones: appendTo equivalent, ability to turn off modifiers, ability to change between aria-describedby, aria-labelledby, and nothing.

  3. we need to try to make this tooltip more accessible than the previous one. for example, hovering away from the trigger and onto the tooltip should not dismiss the tooltip. and pressing Escape should dismiss it. See https://sarahmhigley.com/writing/tooltips-in-wcag-21/

@gretanausedaite
Copy link
Contributor Author

gretanausedaite commented Jun 8, 2023

  1. is it possible to avoid the extra div here? i am going to add an extra div in add portal container to context #1300 so it would be nice if we could just use that instead.

That div comes from FloatingPortal I am using to portal tooltip on given element. I think we could use just one portal (new one). They provide hook (if we need it): https://floating-ui.com/docs/FloatingPortal#usefloatingportalnode

  1. we will definitely need a bunch of customization options. three important ones: appendTo equivalent, ability to turn off modifiers, ability to change between aria-describedby, aria-labelledby, and nothing.

We can easily override aria props. I added appendTo which goes to portal element.
TooltipContent:
image

Copy link
Contributor Author

@gretanausedaite gretanausedaite left a comment

Choose a reason for hiding this comment

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

@mayank99 I think this is in a good shape for review.


const context = data.context;

const hover = useHover(context);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be allow users to customise interactions? eg. disable tooltip on focus.

Copy link
Contributor

Choose a reason for hiding this comment

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

it would be nice to give control, but i fear that developers will start removing focus as trigger, making tooltips inaccessible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not giving control for now. We can add that later.

autoUpdate(referenceEl, floatingEl, update, {
animationFrame: followTrigger,
}),
middleware: [offset(5), flip(), shift()],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Allow users to customise middleware?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe adding prop would be ok?

middleware?: {
  offset?: number;
  flip?: boolean;
  ..etc;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, lets control all the names now. Other option would be to allow user to pass any and all middleware, but then they might start doing weird things

I also think it should be possible for the user to toggle autoUpdate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

autoUpdateOptions were added.

Copy link
Contributor

@mayank99 mayank99 left a comment

Choose a reason for hiding this comment

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

LGTM. 🚀

One more thing I would like to see is the ability to switch between aria-describedby vs aria-labelledby vs no aria. This will allow us to use <Tooltip> inside <IconButton>. But it can be done in a future PR.

Another thing we should do is add FloatingDelayGroup inside ButtonGroup. Again can be done in a future PR.

LostABike

This comment was marked as resolved.

@mayank99

This comment was marked as outdated.

@LostABike

This comment was marked as resolved.

.changeset/silly-roses-yell.md Outdated Show resolved Hide resolved
gretanausedaite and others added 2 commits June 30, 2023 09:08
Co-authored-by: Annie D <22462084+LostABike@users.noreply.github.com>
@gretanausedaite gretanausedaite added this pull request to the merge queue Jun 30, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 30, 2023
@gretanausedaite gretanausedaite added this pull request to the merge queue Jun 30, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Jun 30, 2023
@mayank99 mayank99 added this pull request to the merge queue Jun 30, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 30, 2023
@mayank99 mayank99 added this pull request to the merge queue Jun 30, 2023
Merged via the queue into dev with commit 4375142 Jun 30, 2023
16 of 17 checks passed
@mayank99 mayank99 deleted the greta/flaoting-ui branch June 30, 2023 13:26
@imodeljs-admin imodeljs-admin mentioned this pull request Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v3: Migrate from tippyjs to floating-ui
3 participants