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

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

Open
wants to merge 4 commits into
base: canary
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 3 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)
28 changes: 28 additions & 0 deletions packages/components/modal/__tests__/modal.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {render, fireEvent} from "@testing-library/react";
import userEvent from "@testing-library/user-event";

import {Modal, ModalContent, ModalBody, ModalHeader, ModalFooter} from "../src";
import {Button} from "../../button/src";
wingkwong marked this conversation as resolved.
Show resolved Hide resolved

// 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,33 @@ describe("Modal", () => {
expect(onClose).toHaveBeenCalled();
});

test("should fire 'onOpenChange' callback when open button is clicked", async () => {
sanuj21 marked this conversation as resolved.
Show resolved Hide resolved
const onOpen = jest.fn();

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

const openButton = getByLabelText("Open");

const user = userEvent.setup();

await user.click(openButton);

expect(onOpen).toHaveBeenCalled();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

⚠️ Potential issue

ModalProps interface missing onOpenChange prop

The ModalProps interface does not include the onOpenChange prop, which is used in various Modal components and tests. This inconsistency can lead to type errors and unreliable test behavior.

🔗 Analysis chain

Improve test case for modal opening behavior

While the new test case is a good addition, there are a few areas that could be improved:

  1. The test doesn't actually open the modal or check its state. Consider updating the Modal to start closed and verify it opens.
  2. The Modal is rendered as always open (isOpen={true}), which doesn't reflect a real-world scenario. It should start closed.
  3. The test name mentions 'onOpenChange', but the mock function is called 'onOpen'. This should be consistent.

Here's a suggested improvement:

test("should fire 'onOpenChange' callback when modal is opened", async () => {
  const onOpenChange = jest.fn();

  const { getByLabelText } = render(
    <>
      <Button aria-label="Open">Open Modal</Button>
      <Modal isOpen={false} onOpenChange={onOpenChange}>
        <ModalContent>
          <ModalHeader>Modal header</ModalHeader>
          <ModalBody>Modal body</ModalBody>
          <ModalFooter>Modal footer</ModalFooter>
        </ModalContent>
      </Modal>
    </>
  );

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

  await user.click(openButton);

  expect(onOpenChange).toHaveBeenCalledWith(true);
});

This version:

  • Uses a consistent name for the callback (onOpenChange)
  • Starts with the modal closed
  • Verifies that onOpenChange is called with true when the modal is opened

To ensure the Modal component behaves as expected with these changes, we can run the following verification:

This will confirm that the Modal component accepts an onOpenChange prop of the correct type.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify the Modal component's onOpenChange prop
ast-grep --lang typescript --pattern 'interface ModalProps {
  $$$
  onOpenChange?: (isOpen: boolean) => void;
  $$$
}'

Length of output: 121

Script:

#!/bin/bash
# Search for occurrences of 'onOpenChange' in the codebase
rg 'onOpenChange' --type=typescript

Length of output: 70

Script:

#!/bin/bash
# Search for similar prop names related to opening the modal
rg 'onOpen[^A-Z]' --type=typescript

Length of output: 72

Script:

#!/bin/bash
# Verify the Modal component's onOpenChange prop with a flexible pattern
ast-grep --lang typescript --pattern 'interface ModalProps {
  $$$
  onOpenChange?($_): void;
  $$$
}'

Length of output: 104

Script:

#!/bin/bash
# Search for occurrences of 'onOpenChange' in .ts and .tsx files
rg 'onOpenChange' --glob '*.ts' --glob '*.tsx'

Length of output: 9539

Script:

#!/bin/bash
# Search for similar prop names related to opening the modal in .ts and .tsx files
rg 'onOpen[^A-Z]' --glob '*.ts' --glob '*.tsx'

Length of output: 4430


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

https://github.com/nextui-org/nextui/blob/8a33eabb2583202fcc8fbc33e8f2ed23bb45f1a4/packages/components/modal/src/use-modal.ts#L124

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