Remove usages of deprecated useElementShouldClose #6464
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6464 +/- ##
=======================================
Coverage 99.43% 99.43%
=======================================
Files 270 270
Lines 10214 10219 +5
Branches 2416 2416
=======================================
+ Hits 10156 10161 +5
Misses 58 58 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
I understand the rationale for decomposing useElementShouldClose originally, but perhaps it would be worth having a simple wrapper that encapsulates the "standard" set of actions that can close popover-like controls. In future perhaps we could make these components actually be a native popover, but that's a more substantial refactoring.
| // Interactions outside the component when it is open should close it | ||
| useClickAway(shareRef, closePanel, { enabled: isOpen }); | ||
| useFocusAway(shareRef, closePanel, { enabled: isOpen }); | ||
| useKeyPress(['Escape'], closePanel, { enabled: isOpen }); |
There was a problem hiding this comment.
hypothesis/frontend-shared#900 documented the rationale for decomposing the useElementShouldClose hook into separate hooks, but I think it would be useful to have a combined hook that captured the set of actions that close popover-type controls.
There was a problem hiding this comment.
Yep, maybe you are right. Perhaps what we should do is keep useElementShouldClose, un-deprecate it, and just make it a wrapper of the other three hooks.
What do you think?
There was a problem hiding this comment.
I think I would add a new usePopoverShouldClose hook that wraps the existing hooks and is more specific about the kind of widget it works with. The problem with useElementShouldClose was that we used it for popover-type controls but also different controls that needed different close behaviors.
There was a problem hiding this comment.
Perfect. I'll close this one for now
Replace usages of the deprecated
useElementShouldClosehook from frontend-shared withuseKeyPress(['Escape']),useFocusAwayanduseClickAway, which is the recommended migration path.Some tests needed to be adjusted, as the internal implementation of
useFocusAwayhas been changed after it was extracted fromuseElementShouldClose, and it currently differs from it.