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: Add new hideOnClick
prop
#54406
Conversation
Flaky tests detected in 7b75dc9905d657f8b83030257c1b7bbbdeef565c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6166668891
|
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.
Looks good to me! And thank you for adding unit tests 🚀
Let's add a CHANGELOG entry before merging.
@@ -310,4 +310,26 @@ describe( 'Tooltip', () => { | |||
await screen.findByRole( 'button', { description: 'tooltip text' } ) | |||
).toBeInTheDocument(); | |||
} ); | |||
|
|||
it( 'should not hide tooltip when the anchor is clicked if hideOnClick is false', async () => { |
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.
Might be useful to have a test to confirm that if hideOnClick
is true
or not specified, the tooltip still gets hidden on anchor click.
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 have a test that is kind-of implicitly testing for that
gutenberg/packages/components/src/tooltip/test/index.tsx
Lines 85 to 97 in bfe8550
it( 'should not show tooltip on focus as result of mouse click', async () => { | |
const user = userEvent.setup(); | |
render( <Tooltip { ...props } /> ); | |
await user.click( screen.getByRole( 'button', { name: /Button/i } ) ); | |
expect( | |
screen.queryByRole( 'tooltip', { name: /tooltip text/i } ) | |
).not.toBeInTheDocument(); | |
await cleanupTooltip( user ); | |
} ); |
Although I'd understand if you preferred having a more explicit check.
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.
I guess we can also expand the test description if we want, but yeah, that test does the job IMHO 👍
7b75dc9
to
d053201
Compare
What?
While looking into replacing the
ui/tooltip
component, I came across a feature that isn't currently possible with our Tooltip component, due to legacy behavior where the tooltip is hidden on anchor click. This implementation ofui/tooltip
can be found in ColorPicker and the feature by using the copy button:Why?
To make a more dynamic tooltip possible, like in the case of ColorPicker's copy button:
gutenberg/packages/components/src/color-picker/color-copy-button.tsx
Lines 61 to 63 in 0cf7898
How?
To avoid a regression, an optional prop of
hideOnClick
with a default oftrue
has been added. By default, it will behave the same as it does now, but there will be an option to hide the tooltip when the anchor. This will allow for possibilities like the abovementioned example.A test has been added to test the
false
case to ensure the behavior is as expected.Testing Instructions
npm run test:unit packages/components/src/tooltip/test/index.tsx
npm run storybook:dev
hideOnClick
is set totrue
and stays visible whenfalse