Skip to content

fix(Popover): unnecesary call of onOpen and missing call of onClose #4230

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.

Sign up for GitHub

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

Merged
Barsnes merged 5 commits into from
Nov 24, 2025
Merged

fix(Popover): unnecesary call of onOpen and missing call of onClose #4230

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7c0f47f
test: popover onOpen / onClose with controlled state
unekinn Nov 11, 2025
dfdee89
fix(Popover): don't call onOpen when already open, and call onClose w…
unekinn Nov 11, 2025
1ade00d
Merge branch 'main' into fix/popover-onOpen-when-open
Barsnes Nov 21, 2025
45e5037
Merge branch 'main' into fix/popover-onOpen-when-open
Barsnes Nov 21, 2025
4ab3095
rework ifs
Barsnes Nov 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .changeset/stupid-birds-read.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
"@digdir/designsystemet-react": patch
---

**Popover**: Fix unnecesary call of `onOpen` and missing call of `onClose`

- Don't call `onOpen` when clicking `Popover.Trigger` when `Popover` is already open.
- Call `onClose` when a controlled `Popover` is closed by clicking on `Popover.Trigger`.
9 changes: 7 additions & 2 deletions packages/react/src/components/popover/popover.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,8 +188,13 @@ export const Controlled: StoryFn<typeof Popover> = () => {

return (
<Popover.TriggerContext>
<Popover.Trigger onClick={() => setOpen(!open)}>Slett</Popover.Trigger>
<Popover open={open} onClose={() => setOpen(false)} data-color='neutral'>
<Popover.Trigger>Slett</Popover.Trigger>
<Popover
open={open}
onOpen={() => setOpen(true)}
onClose={() => setOpen(false)}
data-color='neutral'
>
<Paragraph>Er du sikker på at du vil slette?</Paragraph>
<div
style={{
Expand Down
89 changes: 89 additions & 0 deletions packages/react/src/components/popover/popover.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,38 @@ const CompControlled = () => {
);
};

type ControlledWithTriggerProps = {
isInitiallyOpen: boolean;
onOpen?: () => void;
onClose?: () => void;
};

const CompControlledWithTrigger = (props: ControlledWithTriggerProps) => {
const [open, setOpen] = useState(props.isInitiallyOpen);

return (
<Popover.TriggerContext>
<Popover.Trigger>
Dropdown
{open ? <ChevronDownIcon aria-hidden /> : <ChevronUpIcon aria-hidden />}
</Popover.Trigger>
<Popover
open={open}
onOpen={() => {
props.onOpen?.();
setOpen(true);
}}
onClose={() => {
props.onClose?.();
setOpen(false);
}}
>
{contentText}
</Popover>
</Popover.TriggerContext>
);
};

const contentText = 'popover content';

const render = async (props: PopoverProps = {}) => {
Expand All @@ -41,6 +73,19 @@ const render = async (props: PopoverProps = {}) => {
};
};

const renderControlledWithTrigger = async (
props: ControlledWithTriggerProps,
) => {
/* Flush microtasks */
await act(async () => {});
const user = userEvent.setup();

return {
user,
...renderRtl(<CompControlledWithTrigger {...props} />),
};
};

describe('Popover', () => {
it('should render popover on trigger-click when closed', async () => {
const { user } = await render();
Expand Down Expand Up @@ -177,4 +222,48 @@ describe('Popover', () => {
await act(async () => await user.click(document.body));
expect(onClose).toHaveBeenCalledTimes(1);
});

it('should not call onOpen when the popover is open', async () => {
const onOpen = vi.fn();
const { user } = await render({ onOpen });
const popoverTrigger = screen.getByRole('button');

await act(async () => await user.click(document.body));
expect(onOpen).toHaveBeenCalledTimes(0);

await act(async () => await user.click(popoverTrigger));
expect(screen.queryByText(contentText)).toBeVisible();
expect(onOpen).toHaveBeenCalledTimes(1);
await act(async () => await user.click(popoverTrigger));
expect(onOpen).toHaveBeenCalledTimes(1);
});

describe('with controlled state', () => {
it('should not call onClose when the popover is closed', async () => {
const onClose = vi.fn();
const { user } = await renderControlledWithTrigger({
isInitiallyOpen: false,
onClose,
});
const popoverTrigger = screen.getByRole('button');

await act(async () => await user.click(popoverTrigger));
expect(onClose).toHaveBeenCalledTimes(0);
await act(async () => await user.click(popoverTrigger));
expect(onClose).toHaveBeenCalledTimes(1);
});

it('should not call onOpen when the popover is open', async () => {
const onOpen = vi.fn();
const { user } = await renderControlledWithTrigger({
isInitiallyOpen: true,
onOpen,
});
const popoverTrigger = screen.getByRole('button');

expect(screen.queryByText(contentText)).toBeVisible();
await act(async () => await user.click(popoverTrigger));
expect(onOpen).toHaveBeenCalledTimes(0);
});
});
});
11 changes: 8 additions & 3 deletions packages/react/src/components/popover/popover.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,10 +131,15 @@ export const Popover = forwardRef<HTMLDivElement, PopoverProps>(

if (isTrigger) {
event.preventDefault(); // Prevent native Popover API
setInternalOpen((open) => !open);
onOpen?.();
if (!controlledOpen) {
Copy link
Member

@Barsnes Barsnes Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick, but to make it more readable, could we swap the ifs, so we can do if (controlledOpen)?

Copy link
Contributor Author

@unekinn unekinn Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?.();
        }

Copy link
Contributor Author

@unekinn unekinn Nov 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?.();
        }
      };

Copy link
Member

@Barsnes Barsnes Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Liked your last suggestion, went ahead and added it to the PR

setInternalOpen(true);
onOpen?.();
} else {
setInternalOpen(false);
onClose?.();
}
}
if (isOutside && internalOpen) {
if (isOutside && controlledOpen) {
setInternalOpen(false);
onClose?.();
}
Expand Down
Loading