fix: controlled tooltips are not close properly#6418
Conversation
| 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 |
| } | ||
| }; | ||
|
|
||
| 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
| }); | ||
|
|
||
| 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
snowystinger
left a comment
snowystinger
left a comment
There was a problem hiding this comment.
Thanks so much for writing the test!
| } | ||
| }; | ||
|
|
||
| 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 |
LFDanLu
left a comment
LFDanLu
left a comment
There was a problem hiding this comment.
Verified the behavior before and after the fix, LGTM thanks!
|
## 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 |
6 participants
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: