-
Notifications
You must be signed in to change notification settings - Fork 1.6k
fix: hide unmeasured tooltips during exit #10502
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -381,4 +381,16 @@ describe('Tooltip', () => { | |
| let button = getByRole('button'); | ||
| expect(button).toHaveAttribute('tabindex', '0'); | ||
| }); | ||
|
|
||
| it('hides unmeasured tooltips during exit animation', () => { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test is pretty specific to the implementation, probably better to reproduce the actual user flow in the test with mocked timers if possible
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense. I agree the test should cover the actual focus flow rather than asserting the |
||
| let {getByRole} = render( | ||
| <TooltipTrigger isOpen={false}> | ||
| <Button>Trigger</Button> | ||
| <Tooltip isExiting>Unmeasured tooltip</Tooltip> | ||
| </TooltipTrigger> | ||
| ); | ||
|
|
||
| let tooltip = getByRole('tooltip', {hidden: true}); | ||
| expect(tooltip).toHaveStyle('visibility: hidden'); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. expect.toBeVisible
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if we do the either of the suggestions I had instead, we could instead listen to onOpenChange in TooltipTrigger and assert it was never called |
||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
definitely an interesting approach
some other approaches I'd had:
I'm not yet sure which approach I like best, so it's a question for the team.