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.

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

Feat / modal manager #485

Closed
wants to merge 2 commits into from
Closed

Conversation

ChristiaanScheermeijer
Copy link
Collaborator

Description

This PR implements a ModalProvider and useModal hook that manages the visible modals when registered via the useModals hook.

By doing so, the following features are added:

  • Close specific or all open modals using the context (Native/remote back button)
  • Open multiple modals without interfering with each other
  • Keep the initial focus of the first modal opened in case of open (1) -> close (1) and open(2) -> close(2)
  • Handle HTMLElement#inert and body scrolling from a single place
  • Escape key only closes the last opened modal

This change removes lots of duplicate code from the Modal and Sidebar components.

See the whole discussion and review here: Videodock#167

Copy link

github-actions bot commented Apr 5, 2024

Visit the preview URL for this PR (updated for commit a8d4d80):

https://ottwebapp--pr485-feat-modal-manager-24vtofrp.web.app

(expires Fri, 10 May 2024 10:18:09 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: c198f8a3a199ba8747819f7f1e45cf602b777529

useEffect(() => {
// react when the `open` prop changes
openModalEvent();
}, [open, openModalEvent]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you think it could be valuable if we use open or show just for the initial render (and assume the default value is true). If it is false by default, then we can call openModal method from the context later with some readable id. In this case there is no limitation that we can open / close modal window just from the modal itself:

<Sidebar containerRef={containerRef} id="sidebar" />

Inner hook for Modal / Dialog / Alert:

export const useInnerModal = ({ modalId, show = true, onClose, modalRef, containerRef }: Params) => {
  const { openModal, closeModal, modals, registerModal, removeModal } = useContext(ModalContext);
  const isOpen = modals.some((modal) => modal.modalId === modalId);
    
  useEffect(() => {
    registerModal(modalId, modalRef, onClose, containerRef); // can be separate state of registered entities. When we register a modal, we also check that id is unique.
  
    if (show) {
        openModal(modalId);
    }
    
    return () => removeModal(modalId); 
  }, []);
  
  return {
    isOpen,
    closeModal,
    openModal,
  };
}

Additionally we can have one more hook just to be able to use modal actions from the context (we can also address the context directly):

export const useModalActions = () => {
  const { openModal, closeModal } = useContext(ModalContext);

   return {openModal, closeModal}
  });

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

While creating the ModalManager, at one point, I had it exactly like that. But after a while, I felt that most logic in the ModalProvider became much more complex due to the following:

  • The order of opening matters. To close the last opened modal, the stack needs to be sorted or time-stamped
  • Currently, it's a bit funky that both states control the open state, I'd prefer to have a single state responsible for the open state, but this would mean a big refactor of the internal state and click actions (it's doable though)
  • When we add the possibility to open modals using the context, we need to pass an onOpen callback so the state can be updated

Since we currently don't really need the benefit of opening modals using the context, I would like to keep this PR simple.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking about having two stacks: 1 for registered windows with ids (or simply dictionary), 1 for opened windows. This way second one will behave the same way it does now. I am not sure about onOpen callback. In case we use openModal, then isOpen will be updated automatically.

My main concern is actually this part where we use several open states:

  const isOpen = modals.some((modal) => modal.modalId === modalId);

  // replace with React `useEffectEvent` when stable: https://react.dev/reference/react/experimental_useEffectEvent
  const openModalEvent = useEventCallback(() => {
    if (open) {
      openModal(modalId, modalRef, onClose, containerRef);
    } else if (isOpen) {
      closeModal(modalId, false);
    }
  });

and this one where we allow control via incoming props:

  useEffect(() => {
    // react when the `open` prop changes
    openModalEvent();
  }, [open, openModalEvent]);

So we have different ways now of how we close and open modals:

  1. We open them using open prop.
  2. We close them using callbacks (both for open and close). And we can't do it by settings open to false.

Should we just use open / show prop to control the state? This way no methods will be exposed from the hook.

});
};

let originFocusElement: HTMLElement | null = null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this one be part of the internal context state?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not needed in the context, and I want to prevent unnecessary state updates (especially in the useEffect that sets this variable).

@ChristiaanScheermeijer
Copy link
Collaborator Author

Superseded by #505

@AntonLantukh
Copy link
Collaborator

@ChristiaanScheermeijer I will close this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants