Pomax / react-onclickoutside

An onClickOutside wrapper for React components
MIT License
1.83k stars 187 forks source link

Problem when using hooks + multiple components #329

Open thismarcoantonio opened 5 years ago

thismarcoantonio commented 5 years ago

Hi everyone,

I'm having a problem, I've been using this library for a while working 100%. Today I tried to implement it in another part of my code, and it just don't work properly. I've tried this in version 6.8.0 and 6.9.0.

Here's my code:

import React from "react"
import onClickOutside from "react-onclickoutside"

function FocusHandler({ children }) {
  const [hasFocus, setFocus] = React.useState(false)
  FocusHandler.handleClickOutside = () => setFocus(false)

  return (
    <div style={{ width: "100%" }} onFocus={() => setFocus(true)}>
      {children({ hasFocus, setFocus })}
    </div>
  )
}

const clickOutsideConfig = {
  handleClickOutside: () => FocusHandler.handleClickOutside
}

export default onClickOutside(FocusHandler, clickOutsideConfig)

If I only use this in one component, everything works properly. If I two or more components, it breaks.

EDIT: Also made a simple example: https://codesandbox.io/embed/react-onclickoutside-bug-report-jc963

Pomax commented 5 years ago

This HOC works based on the assumption that it wraps true instances, so if you use a functional component, and you only use one of them on a page, it will likely work, but if you use more than one, the last one "wins" and is the only one that'll work because the FocusHandler gets treated as an instance, which as singleton function object, will definitely go wrong.

thismarcoantonio commented 5 years ago

I see... there is any way to get around that? Should I use a new HOC for every component? Why the functional difference matters here?

elya158-zz commented 5 years ago

I have this issue as well, will there be a fix for this?

Quocnamit commented 5 years ago

can we use class component to fix it ?

Quocnamit commented 5 years ago

https://codesandbox.io/s/loving-newton-754t8 I try to use the class component. so the event can apply to multiples components

cypherfunc commented 5 years ago

FocusHandler.handleClickOutside = () => setFocus(false)

This line is the problem. This ties a static method to a single instance, which is a serious anti-pattern in React.

The class-based usage is fine, because it uses an instance method instead of a static method.

thismarcoantonio commented 5 years ago

Makes sense, don't know why I didn't realised about it before...

abominab commented 5 years ago

The linked example in this section (https://github.com/Pomax/react-onclickoutside#functional-component-with-usestate-hook) will demonstrate this bug if you put 2 Menu components in index.js Would be nice if this got fixed internally so that we can use this w/ functional components.

strongui commented 5 years ago

Is this something that will be addressed in a future release?

metalass commented 4 years ago

Managed to resolve it with introducing unique name (id) for the component. So, imagine we have name prop which value is unique. We'd need to adjust static component property assignment in order to set unique property:

Menu['handleClickOutside_' + name] = () => setIsOpen(false);

And config:

const clickOutsideConfig = {
    handleClickOutside: ({ props }) => Menu['handleClickOutside_' + props.name]
};

Though, this is a dirty workaround, hope it would help someone.

FranciscoMateusVG commented 4 years ago

@metalass you sir deserve a medal! Worked like a charm in "react": "^16.13.1" and "react-onclickoutside": "^6.9.0".

Makwe-O commented 4 years ago

OMG. Thank you for this @metalass. It's a workaround but it definitely works and makes sense!!

iqor commented 4 years ago

Ty @metalass , worked fine on "react": "^17.0.1" and "react-onclickoutside": "^6.9.0"

antal-bukos commented 3 years ago

Using this hook also works https://usehooks.com/useOnClickOutside/.