I have a modal that closes if user clicked outside it.

approach one - passing isModalOpened so the state updates on click only if isModalOpened is true.

const [isModalOpened, toggleModal] = useState(false);
const ref = useRef(null);

const clickOut = (e) => {
  if (!ref.current.contains(e.target) && isModalOpened) {
     toggleModal(false);
  }
};

React.useEffect(() => {
  window.addEventListener('mousedown', clickOut);

  return () => {
     window.removeEventListener('mousedown', clickOut);
  };
}, [isModalOpened]);

approach two - removing the isModalOpened from the dep array.

const [isModalOpened, toggleModal] = useState(false);
const ref = useRef(null);

const clickOut = (e) => {
  if (!ref.current.contains(e.target)) {
     toggleModal(false);
  }
};

React.useEffect(() => {
  window.addEventListener('mousedown', clickOut);

  return () => {
     window.removeEventListener('mousedown', clickOut);
  };
}, []);

Question: should I pass or not to pass the isModalOpened to the dep array?

I have a modal that closes if user clicked outside it.

approach one - passing isModalOpened so the state updates on click only if isModalOpened is true.

const [isModalOpened, toggleModal] = useState(false);
const ref = useRef(null);

const clickOut = (e) => {
  if (!ref.current.contains(e.target) && isModalOpened) {
     toggleModal(false);
  }
};

React.useEffect(() => {
  window.addEventListener('mousedown', clickOut);

  return () => {
     window.removeEventListener('mousedown', clickOut);
  };
}, [isModalOpened]);

approach two - removing the isModalOpened from the dep array.

const [isModalOpened, toggleModal] = useState(false);
const ref = useRef(null);

const clickOut = (e) => {
  if (!ref.current.contains(e.target)) {
     toggleModal(false);
  }
};

React.useEffect(() => {
  window.addEventListener('mousedown', clickOut);

  return () => {
     window.removeEventListener('mousedown', clickOut);
  };
}, []);

Question: should I pass or not to pass the isModalOpened to the dep array?

• javascript
• reactjs

• A little tip if your creating dialogs, don't use window, but instead place a full screen fixed transparent div, or semi transparent div, and attach the events to this. Why?, because it allows you to have dialogs within dialogs... – Keith Commented Nov 14, 2020 at 19:18

4 Answers 4

You do not need it.

The reason being that if you toggleModal to the same false value it will not cause a re-render.

So you do not need to guard against the value of isModalOpened which leads to not including the variable in you function, which leads to not requiring the dependency at all.

No, you shouldn't pass isModalOpen to the deep array because in this case your effect will just remove and add again the listener. It is unnecessary

Approach one will run the hook every time isModalOpened changes, removing the global listener when the modal closes. Approach two will run the hook when the ponent is (un) mounted, meaning the global listener will be active throughout the ponent lifecycle.

The answer depends on how you're planning to use the ponent. I'm guessing you plan to mount the ponent before the modal is opened (hence the false initial state), which means your 2nd approach will listen to the mousedown event from the moment the ponent is mounted to the moment it's unmounted. This approach would be valid if you were planning to mount the ponent only when the modal is opened. But you're not, which means you should set the global listener only when the modal is opened.

1st approach is correct.

*edit

But you can remove the isModalOpened check in your if statement.

I would suggest a bit different approach towards this. I would here wrap the clickOut function in useCallback which have dependencies as toggleModal and ref and have clickOut as dependency in useEffect. In this way whenever you ref or toggleModal changes, you have a new reference for clickOut and if you have new reference for clickOut you listeners will be assigned again in useEffect. This will help you will unnecessary clickOut function creation on each render and optimize the rendering as well.

So your code as per my suggestion will look like this:

const [isModalOpened, toggleModal] = useState(false);
const ref = useRef(null);

const clickOut = useCallback((e) => {
  if (!ref.current.contains(e.target) && isModalOpened) {
     toggleModal(false);
  }
}, [isModalOpened, ref]);

React.useEffect(() => {
  window.addEventListener('mousedown', clickOut);

  return () => {
     window.removeEventListener('mousedown', clickOut);
  };
}, [clickOut]);