fix(Popover): unnecesary call of onOpen and missing call of onClose#4230
fix(Popover): unnecesary call of onOpen and missing call of onClose#4230
onOpen and missing call of onClose#4230Conversation
🦋 Changeset detectedLatest commit: 4ab3095 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Preview deployments for this pull request: storybook - |
7fd7b2d to
cd112c2
Compare
…hen closed via Trigger also when controlled
cd112c2 to
dfdee89
Compare
| event.preventDefault(); // Prevent native Popover API | ||
| setInternalOpen((open) => !open); | ||
| onOpen?.(); | ||
| if (!controlledOpen) { |
There was a problem hiding this comment.
Nitpick, but to make it more readable, could we swap the ifs, so we can do if (controlledOpen)?
There was a problem hiding this comment.
Were you thinking something like this? Not sure if it's actually more readable, but at least we only have the close logic once in this version.
if (controlledOpen) {
if (isTrigger) {
event.preventDefault(); // Prevent native Popover API
}
if (isTrigger || isOutside) {
setInternalOpen(false);
onClose?.();
}
} else if (isTrigger) {
event.preventDefault(); // Prevent native Popover API
setInternalOpen(true);
onOpen?.();
}There was a problem hiding this comment.
Or maybe like this
if (isTrigger) {
event.preventDefault(); // Prevent native Popover API
}
if (controlledOpen && (isTrigger || isOutside)) {
setInternalOpen(false);
onClose?.();
} else if (isTrigger) {
setInternalOpen(true);
onOpen?.();
}
};There was a problem hiding this comment.
Liked your last suggestion, went ahead and added it to the PR
|
I'll make sure to fix this before next release 😄 |
Summary
Popover: Fix unnecesary call of
onOpenand missing call ofonCloseonOpenwhen clickingPopover.TriggerwhenPopoveris already open.onClosewhen a controlledPopoveris closed by clicking onPopover.Trigger.Checks
pnpm changesetif relevant)