Skip to content
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

Jump to bottom

fix(modal): onOpenChange triggering when modal opens #3902

Open
wants to merge 4 commits into
base: canary
Choose a base branch
from
+54 −6
Open

fix(modal): onOpenChange triggering when modal opens #3902

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4741a84
fix: onOpenChange triggering when modal opens
sanuj21 Oct 16, 2024
e4749c6
chore: summarized changeset
sanuj21 Oct 16, 2024
dbf2c49
test: when modal opens onOpenChange is called
sanuj21 Oct 16, 2024
d20d2f2
test: corrected the modal testcase
sanuj21 Oct 16, 2024
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
6 changes: 6 additions & 0 deletions .changeset/early-ghosts-fix.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@nextui-org/modal": patch
"@nextui-org/use-disclosure": patch
---

Added useEffect in useModal to fire onOpenChange and in useDisclosure, onOpenChange now accepts onOpen as parameter(#3887)
35 changes: 34 additions & 1 deletion packages/components/modal/__tests__/modal.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import * as React from "react";
import {render, fireEvent} from "@testing-library/react";
import userEvent from "@testing-library/user-event";
import {Button} from "@nextui-org/button";

import {Modal, ModalContent, ModalBody, ModalHeader, ModalFooter} from "../src";
import {Modal, ModalContent, ModalBody, ModalHeader, ModalFooter, useDisclosure} from "../src";

// e.g. console.error Warning: Function components cannot be given refs.
// Attempts to access this ref will fail. Did you mean to use React.forwardRef()?
Expand Down Expand Up @@ -91,6 +92,38 @@ describe("Modal", () => {
expect(onClose).toHaveBeenCalled();
});

const ModalWrapper = ({onOpenChange}) => {
const {isOpen, onOpen} = useDisclosure();

return (
<>
<Button aria-label="Open" onClick={onOpen}>
Open Modal
</Button>
<Modal isOpen={isOpen} onOpenChange={onOpenChange}>
<ModalContent>
<ModalHeader>Modal header</ModalHeader>
<ModalBody>Modal body</ModalBody>
<ModalFooter>Modal footer</ModalFooter>
</ModalContent>
</Modal>
</>
);
};

test("should fire 'onOpenChange' callback when open button is clicked and modal opens", async () => {
const onOpenChange = jest.fn();

const {getByLabelText} = render(<ModalWrapper onOpenChange={onOpenChange} />);

const openButton = getByLabelText("Open");
const user = userEvent.setup();

await user.click(openButton);

expect(onOpenChange).toHaveBeenCalled();
});

it("should hide the modal when pressing the escape key", () => {
const onClose = jest.fn();

Expand Down
8 changes: 7 additions & 1 deletion packages/components/modal/src/use-modal.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import type {HTMLMotionProps} from "framer-motion";

import {AriaModalOverlayProps} from "@react-aria/overlays";
import {useAriaModalOverlay} from "@nextui-org/use-aria-modal-overlay";
import {useCallback, useId, useRef, useState, useMemo, ReactNode} from "react";
import {useCallback, useId, useRef, useState, useMemo, ReactNode, useEffect} from "react";
import {modal} from "@nextui-org/theme";
import {
HTMLNextUIProps,
Expand Down Expand Up @@ -129,6 +129,12 @@ export function useModal(originalProps: UseModalProps) {
},
});

useEffect(() => {
if (isOpen) {
onOpenChange?.(isOpen);
}
}, [isOpen]);
ryo-manba marked this conversation as resolved.
Show resolved Hide resolved

Comment on lines +132 to +137
Copy link

Choose a reason for hiding this comment

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

I came across this interesting resource on the React docs (https://react.dev/learn/you-might-not-need-an-effect#adjusting-some-state-when-a-prop-changes) that suggests alternative approaches to useEffect in some scenarios. It might be worth considering if it applies to this particular use case.

In the example provided, they achieve a similar outcome by storing information from previous renders. I took a stab at adapting it here (untested):

const [prevIsOpen, setPrevIsOpen] = useState(isOpen);
if (isOpen !== prevIsOpen) {
  setPrevIsOpen(isOpen);
  if (isOpen) {
    onOpenChange?.(isOpen);
  }
}

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, It seems a very nice idea to avoid useEffect. I'll test and see if it works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested, actually the onOpenChange is eventually changing the isOpen state. And we shouldn't directly update parent state inside child without useEffect.

I tried though and got the warning : "Cannot update a component (App) while rendering a different component (NextUI.Modal)".

Let me know if I am missing something.

const {modalProps, underlayProps} = useAriaModalOverlay(
{
isDismissable,
Expand Down
11 changes: 7 additions & 4 deletions packages/hooks/use-disclosure/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,14 @@ export function useDisclosure(props: UseDisclosureProps = {}) {
onOpenPropCallbackRef?.();
}, [isControlled, onOpenPropCallbackRef]);

const onOpenChange = useCallback(() => {
const action = isOpen ? onClose : onOpen;
const onOpenChange = useCallback(
(isOpen: boolean) => {
const action = isOpen ? onOpen : onClose;
Comment on lines +47 to +49
Copy link
Member

Choose a reason for hiding this comment

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

Since isOpen is managed within useDisclosure whether it is controlled or uncontrolled, is it necessary to pass it as an argument?

Copy link
Contributor Author

@sanuj21 sanuj21 Oct 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, in this case its needed. Because when the onClose is called its modifiying the internal state as I said earlier ( isOpen becomes false), but at this point the isOpen which is in useDisclosure is still true.
Initially the definition of onOpenChange was like this isOpen ? onClose : onOpen, thats why it was working when onClose used to get called.
But now that will cause infinite render due to useEffect, thats why I reversed it and and added and argument.

Copy link
Member

Choose a reason for hiding this comment

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

Isn't that the expected behavior?

  1. You press the close button → state.close (internal state becomes false).
  2. onOpenChange is called (at this point, useDisclosure state is still true).
  3. Since isOpen is true, onClose gets called.
  4. The useDisclosure state becomes false (syncing the internal state with useDisclosure).

Accepting an argument would be a breaking change, so it might be better to leave it as is for backward compatibility.

Copy link
Member

Choose a reason for hiding this comment

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

Could you clarify in what cases the useEffect bug occurs?

Copy link
Contributor Author

@sanuj21 sanuj21 Oct 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Isn't that the expected behavior?

  1. You press the close button → state.close (internal state becomes false).
  2. onOpenChange is called (at this point, useDisclosure state is still true).
  3. Since isOpen is true, onClose gets called.
  4. The useDisclosure state becomes false (syncing the internal state with useDisclosure).

Accepting an argument would be a breaking change, so it might be better to leave it as is for backward compatibility.

  1. You press the open button → external state becomes true. That will remount modal Component.

  2. useOverlayTriggerState will re-run with isOpen true. After this, useEffect will be called and it will call the outer onOpenChange(which was received as prop to modal), without passing argument(( useDisclosure state is true here).

  3. Since isOpen is true, onClose gets called(at this point, useDisclosure state becomes false, but internal state is still true). (Not sync)

  4. Now, you press the close button → state.close (internal state becomes false).

  5. onOpenChange is called (at this point, useDisclosure state is false), which means onOpen gets called but here onClose needs to get called.

Ya, i thought that it might be a breaking change, but we are also passing state while calling onOpenChange from useOverlayTriggerState

nextui/packages/components/modal/src/use-modal.ts

Line 124 in 8a33eab

onOpenChange: (isOpen) => {

Copy link
Member

Choose a reason for hiding this comment

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

Oh, so the useEffect added in useModal is the issue. Hmm, with the current implementation, it might introduce new bugs. Could you consider exploring alternative implementation methods?

Copy link
Member

Choose a reason for hiding this comment

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

What do you think? @jrgarciadev


action();
}, [isOpen, onOpen, onClose]);
action();
},
[isOpen, onOpen, onClose],
);

return {
isOpen: !!isOpen,
Expand Down