-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
ToolTip: refactor using ariakit #48440
Changes from 55 commits
73f1884
488f4c9
f69c2c8
ab1bc4e
60ee046
16d2d12
1aebcf9
0d20541
6d0eb21
cfb6c4a
32d654f
5386d21
53fd2cd
adc88cb
b18b13a
9ffb2c9
3e1fcca
324ba78
6f6d495
2eb848a
64062b4
e7088d1
a5c8ff3
5c94f87
e40a31c
60e0ac3
ca2fb50
89ad595
a415651
2207553
52fdc3d
800f258
875d9b5
fef59d6
60ff328
56c353d
4bd9455
ef7d837
814ad63
956bffd
beb9a29
8854357
26059b6
53a1f1c
dc53c00
bb98faf
50b7f5d
75e4ea5
ed4c6aa
cf59cd4
6c54d5c
45ba198
59fec6c
b43abd7
869bdde
968add1
5761d79
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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. The changelog needs to be updated, but I'll wait until the Popover changes are merged, and this PR is updated respectively. |
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. tests were checking |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,7 @@ import { plusCircle } from '@wordpress/icons'; | |
*/ | ||
import Button from '..'; | ||
import Tooltip from '../../tooltip'; | ||
import cleanupTooltip from '../../tooltip/test/utils'; | ||
|
||
jest.mock( '../../icon', () => () => <div data-testid="test-icon" /> ); | ||
|
||
|
@@ -193,7 +194,7 @@ describe( 'Button', () => { | |
|
||
render( <Button icon={ plusCircle } label="WordPress" /> ); | ||
|
||
expect( screen.queryByText( 'WordPress' ) ).not.toBeInTheDocument(); | ||
expect( screen.queryByText( 'WordPress' ) ).not.toBeVisible(); | ||
|
||
// Move focus to the button | ||
await user.tab(); | ||
|
@@ -230,12 +231,14 @@ describe( 'Button', () => { | |
/> | ||
); | ||
|
||
expect( screen.queryByText( 'Label' ) ).not.toBeInTheDocument(); | ||
expect( screen.queryByText( 'Label' ) ).not.toBeVisible(); | ||
|
||
// Move focus to the button | ||
await user.tab(); | ||
|
||
expect( screen.getByText( 'Label' ) ).toBeVisible(); | ||
|
||
await cleanupTooltip( user ); | ||
} ); | ||
|
||
it( 'should populate tooltip with description content for buttons with visible labels (buttons with children)', async () => { | ||
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. Kind of unrelated to this PR, I don't think this test has the right assertion based on the test title. The test queries the button but not the tootlip. I bring this up because I would have thought we'd use the cleanup function here too. But this could be a follow-up PR. |
||
|
@@ -287,12 +290,14 @@ describe( 'Button', () => { | |
<Button icon={ plusCircle } label="WordPress" children={ [] } /> | ||
); | ||
|
||
expect( screen.queryByText( 'WordPress' ) ).not.toBeInTheDocument(); | ||
expect( screen.queryByText( 'WordPress' ) ).not.toBeVisible(); | ||
|
||
// Move focus to the button | ||
await user.tab(); | ||
|
||
expect( screen.getByText( 'WordPress' ) ).toBeVisible(); | ||
|
||
await cleanupTooltip( user ); | ||
} ); | ||
|
||
it( 'should not show the tooltip when icon and children defined', async () => { | ||
|
@@ -321,12 +326,14 @@ describe( 'Button', () => { | |
</Button> | ||
); | ||
|
||
expect( screen.queryByText( 'WordPress' ) ).not.toBeInTheDocument(); | ||
expect( screen.queryByText( 'WordPress' ) ).not.toBeVisible(); | ||
|
||
// Move focus to the button | ||
await user.tab(); | ||
|
||
expect( screen.getByText( 'WordPress' ) ).toBeVisible(); | ||
|
||
await cleanupTooltip( user ); | ||
} ); | ||
} ); | ||
|
||
|
ciampo marked this conversation as resolved.
Show resolved
Hide resolved
|
ciampo marked this conversation as resolved.
Show resolved
Hide resolved
|
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.
We no longer have a custom event catcher