fix: controlled tooltips are not close properly #6418
Conversation
| } else if (!closeTimeout.current) { | ||
| closeTimeout.current = setTimeout(() => { | ||
| closeTimeout.current = null; | ||
| close(); | ||
| closeCallback.current(); |
There was a problem hiding this comment.
no need to change this line of code (it has no bug), but changed it for consistency.
use close callback only from ref
|
Thanks for the PR! Mind signing the Adobe CLA? |
Done |
| @@ -119,6 +120,10 @@ export function useTooltipTriggerState(props: TooltipTriggerProps = {}): Tooltip | |||
| } | |||
| }; | |||
|
|
|||
| useEffect(() => { | |||
There was a problem hiding this comment.
we can use useEffectEvent instead of managing this ref ourselves
There was a problem hiding this comment.
Are you sure, that's a good idea?
From the doc
Effect Events are very limited in how you can use them:
**Only call them from inside Effects.**
Never pass them to other components or Hooks.
we call 'close' not inside effect
There was a problem hiding this comment.
we have one from before those limitations were involved and because we have to support it all the way back to react 16, https://github.com/adobe/react-spectrum/blob/main/packages/%40react-aria/utils/src/useEffectEvent.ts which I think is doing the same thing as you've done here
but no worries, can replace later
|
Also, if it's not too hard, would you mind adding a unit test? |
igorman007
force-pushed
the
fixUseTooltipControlledMultiple
branch
from
18e1642 to
2782534
Compare
| @@ -196,4 +213,45 @@ describe('useTooltipTriggerState', function () { | |||
| expect(onOpenChange).toHaveBeenCalledWith(false); | |||
| }); | |||
| }); | |||
|
|
|||
| describe('multiple controlled tooltips', () => { | |||
| it('closes previus tooltip when opening a new one', () => { | |||
There was a problem hiding this comment.
this test fails without fix and passes after fix
| @@ -119,6 +120,10 @@ export function useTooltipTriggerState(props: TooltipTriggerProps = {}): Tooltip | |||
| } | |||
| }; | |||
|
|
|||
| useEffect(() => { | |||
There was a problem hiding this comment.
we have one from before those limitations were involved and because we have to support it all the way back to react 16, https://github.com/adobe/react-spectrum/blob/main/packages/%40react-aria/utils/src/useEffectEvent.ts which I think is doing the same thing as you've done here
but no worries, can replace later
|
GET_BUILD |
|
## API Changes
unknown top level export { type: 'any' } |
|
That's great! May I ask when this fix will be published? |
|
It'll be in our next release. Sometime in the next couple weeks most likely |
Closes #6233
✅ Pull Request Checklist:
📝 Test Instructions:
You can check this fix at storybook page.
http://localhost:9003/?path=/story/tooltiptrigger--controlled-multiple-tooltips&providerSwitcher-express=false&strict=true
before: the first tooltip doesn't close, when the second opens
after: the first tooltip closes, when the second opens
🧢 Your Project: