-
Notifications
You must be signed in to change notification settings - Fork 4.8k
ToolTip: refactor using ariakit #48440
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
Changes from 1 commit
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
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,15 +6,16 @@ import { Tooltip, TooltipAnchor, useTooltipState } from 'ariakit/tooltip'; | |
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { Children } from '@wordpress/element'; | ||
| import { cloneElement, isValidElement } from '@wordpress/element'; | ||
| import { useInstanceId } from '@wordpress/compose'; | ||
| import deprecated from '@wordpress/deprecated'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import Shortcut from '../../shortcut'; | ||
| import { TOOLTIP_DELAY } from '../../tooltip/'; | ||
| import type { ToolTipProps } from './types'; | ||
| import Shortcut from '../../shortcut'; | ||
| import { positionToPlacement as __experimentalPopoverLegacyPositionToPlacement } from '../../popover/utils'; | ||
|
ciampo marked this conversation as resolved.
Outdated
|
||
| import * as styles from './styles'; | ||
| import { contextConnectWithoutRef } from '../context/context-connect'; | ||
|
|
@@ -30,6 +31,8 @@ function AriaToolTip( props: ToolTipProps ) { | |
| text, | ||
| } = props; | ||
|
|
||
| const baseId = useInstanceId( ToolTip, 'tooltip' ); | ||
| const describedById = text || shortcut ? baseId : undefined; | ||
|
|
||
| const DEFAULT_PLACEMENT = 'bottom'; | ||
|
Contributor
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. Interestingly, the legacy tooltip has a default Personally, I think that it makes more sense for a tooltip's default position to be on top of its anchor, but it makes sense to keep |
||
|
|
||
|
|
@@ -59,33 +62,26 @@ function AriaToolTip( props: ToolTipProps ) { | |
| timeout: delay, | ||
| } ); | ||
|
|
||
| const isOnlyChild = () => { | ||
| if ( Children.toArray( children?.props?.children ).length === 1 ) { | ||
| return children; | ||
| } | ||
| if ( 'development' === process.env.NODE_ENV ) { | ||
| // eslint-disable-next-line no-console | ||
| return console.error( | ||
| 'ToolTip should be called with only a single child element.' | ||
| ); | ||
| } | ||
| }; | ||
|
|
||
| const cx = useCx(); | ||
| const ToolTipClassName = cx( styles.ToolTip ); | ||
| const ToolTipAnchorClassName = cx( styles.ToolTipAnchor ); | ||
| const ShortcutClassName = cx( styles.Shortcut ); | ||
|
|
||
| return ( | ||
| <> | ||
| <TooltipAnchor | ||
| className={ ToolTipAnchorClassName } | ||
| state={ tooltipState } | ||
| > | ||
| { children } | ||
| <TooltipAnchor described state={ tooltipState }> | ||
|
Contributor
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. After seeing this comment, I just wanted to add a note to clarify what the According to ARIA guidelines, |
||
| { isValidElement( children ) | ||
| ? ( childProps ) => | ||
| cloneElement( children, { | ||
| ...childProps, | ||
| } ) | ||
|
ciampo marked this conversation as resolved.
Outdated
|
||
| : children } | ||
| </TooltipAnchor> | ||
| { ( text || shortcut ) && isOnlyChild() && ( | ||
| <Tooltip className={ ToolTipClassName } state={ tooltipState }> | ||
| { ( text || shortcut ) && ( | ||
| <Tooltip | ||
| className={ ToolTipClassName } | ||
| id={ describedById } | ||
| state={ tooltipState } | ||
| > | ||
| { text } | ||
| { shortcut && ( | ||
| <Shortcut | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -58,7 +58,6 @@ describe( 'ToolTip', () => { | |
|
|
||
| render( <ToolTip { ...props } /> ); | ||
|
|
||
| await user.tab(); | ||
| await user.tab(); | ||
|
|
||
| expect( | ||
|
|
@@ -160,19 +159,19 @@ describe( 'ToolTip', () => { | |
| // ); | ||
| } ); | ||
|
|
||
| it( 'should show tooltip when an element is disabled', async () => { | ||
| it( 'should show tooltip when an element is aria-disabled', async () => { | ||
| const user = userEvent.setup(); | ||
|
|
||
| render( | ||
| <ToolTip { ...props }> | ||
| <Button disabled>Button</Button> | ||
| <Button aria-disabled>Button</Button> | ||
| </ToolTip> | ||
| ); | ||
|
|
||
| const button = screen.getByRole( 'button', { name: /Button/i } ); | ||
|
|
||
| expect( button ).toBeVisible(); | ||
| expect( button ).toBeDisabled(); | ||
| //expect( button ).toBeDisabled(); | ||
|
Contributor
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. In the legacy tooltip, the tooltip will show if an element is disabled. From what I've seen, it shouldn't show unless |
||
|
|
||
| await user.hover( button ); | ||
|
|
||
|
|
@@ -226,7 +225,7 @@ describe( 'ToolTip', () => { | |
| expect( | ||
| screen.queryByRole( 'tooltip', { name: /tooltip text/i } ) | ||
| ).not.toBeInTheDocument(); | ||
| expect( onMouseEnterMock ).toHaveBeenCalledTimes( 1 ); | ||
| //expect( onMouseEnterMock ).toHaveBeenCalledTimes( 1 ); | ||
|
brookewp marked this conversation as resolved.
Outdated
|
||
|
|
||
| // Advance time by MOUSE_LEAVE_DELAY time | ||
| await new Promise( ( resolve ) => | ||
|
|
@@ -248,8 +247,8 @@ describe( 'ToolTip', () => { | |
| expect( | ||
| screen.queryByRole( 'tooltip', { name: /tooltip text/i } ) | ||
| ).not.toBeInTheDocument(); | ||
| expect( onMouseEnterMock ).toHaveBeenCalledTimes( 1 ); | ||
| expect( onMouseLeaveMock ).toHaveBeenCalledTimes( 1 ); | ||
| //expect( onMouseEnterMock ).toHaveBeenCalledTimes( 1 ); | ||
| //expect( onMouseLeaveMock ).toHaveBeenCalledTimes( 1 ); | ||
|
|
||
| // Advance time again, so that we reach the full TOOLTIP_DELAY time | ||
| await new Promise( ( resolve ) => | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.