Pomax / react-onclickoutside

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

handleClickOutside not bound when using functional Components with hooks #307

Closed ZeroPie closed 5 years ago

ZeroPie commented 5 years ago

Example:

const Menu = ({ userData }) => {
    const [isOpen, setIsOpen] = useState(false)
    const toggle = () => setIsOpen(!isOpen)
    const handleClickOutside = () => setIsOpen(false)

     return (
        <li className={isOpen ? "m-menu -active" : "m-menu "} onClick={toggle}>
...
        )
}
export default onClickOutside(Menu);

Edit vn66kq7mml

Error:


react-onclickoutside.es.js:184 Uncaught Error: WrappedComponent lacks a handleClickOutside(event) function for processing outside click events.
    at onClickOutside._this.__outsideClickHandle```
Pomax commented 5 years ago

Hm, I don't see an immediate way to fix that, since the only two ways the HOC currently supports event handling is by either calling the handler that is part of the component's class, or by you explicitly saying what it as an options object, as second argument to onClickOutside(Menu, {...}).

What happens if you do this:

const Menu = ({ userData }) => {
    const [isOpen, setIsOpen] = useState(false)
    const toggle = () => setIsOpen(!isOpen)
    const handleClickOutside  = () => setIsOpen(false);
    const ret = <li className={isOpen ? "m-menu -active" : "m-menu "} onClick={toggle}>...</li>;

    // explicitly bind handler to the React-managed object:
    ret.handleClickOutside = handleClickOutside;

    return ret;
}
export default onClickOutside(Men
ZeroPie commented 5 years ago

Hi, thanks for the answer: Alas, the return Object is not extensible

TypeError: Cannot add property handleClickOutside, object is not extensible

Simplified CodePen with the problem and your proposed solution: Edit vn66kq7mml

Pomax commented 5 years ago

After a bit of puzzling, this will work:

import React, { useState } from "react";
import "./Menu.css";
import onClickOutside from "react-onclickoutside";

const Menu = () => {
  const [isOpen, setIsOpen] = useState(false);
  const toggle = () => setIsOpen(!isOpen);

  // There are no instances, only the single Menu function, so if we
  // need "properties" that are externally accessible, we'll need to
  // set them on the Menu function itself:
  Menu.handleClickOutside = () => setIsOpen(false);

  return (
    <li className={isOpen ? "m-menu -active" : "m-menu "} onClick={toggle}>
      <div> Open Menu </div>
      <ul className="m-menu__list">
        <li className="m-menu__item">
          <div className="m-menu__link">Log Out</div>
        </li>
      </ul>
    </li>
  );
};

var clickOutsideConfig = {
  handleClickOutside: function(instance) {
    // There aren't any "instances" when dealing with functional
    // components, so we ignore the instance parameter entirely,
    //  and just return the handler that we set up for Menu:
    return Menu.handleClickOutside;
  }
};

export default onClickOutside(Menu, clickOutsideConfig);
ZeroPie commented 5 years ago

Sweet, it works. Thanks for the comments in the code, i understand now. And thank you a lot for your time!!

Btw: Maybe it is a good idea to add a little snippet in the Readme like the following one:

Usage with Hooks and Functional Components

React, { useState } from "react";
import onClickOutside from "react-onclickoutside";

const Menu = () => {
  const [isOpen, setIsOpen] = useState(false);
  const toggle = () => setIsOpen(!isOpen);
  Menu.handleClickOutside = () => setIsOpen(false);
  return (
      .../
  )
};

const clickOutsideConfig = {
  handleClickOutside: () => Menu.handleClickOutside
};

export default onClickOutside(Menu, clickOutsideConfig);
koreyboone commented 5 years ago

If for some reason you had multiple menus I don't believe this would work. It would only update the last one.

Pomax commented 5 years ago

Once you need multiple menus, generally you turn your functional component into a real class, because now you're dealing with a reusable component where it's important that there's some kind of this to tap into for event handling/filtering.

Pomax commented 5 years ago

@ZeroPie sounds like a good idea, would you be interested in stubbing out a PR for that?

ZeroPie commented 5 years ago

PR: https://github.com/Pomax/react-onclickoutside/pull/308 :) thanks!

gpietro commented 5 years ago

There is a way to access the state of the instance? I'm rewriting react-numpad with hooks but I'm struggling to make it work with onClickOutside. https://gist.github.com/gpietro/67c9b2638421224671876df2dd59abfa#file-keypad-js-L133

jmattheis commented 5 years ago

Anything speaking against using a wrapper class?

import * as React from "react";
import onClickOutside from "react-onclickoutside";

interface OnClickOutsideProps {
    onClickOutside: (event) => void;
}

export class XOutsideClickHandler extends React.Component<OnClickOutsideProps>{
    public handleClickOutside = (event) => this.props.onClickOutside(event);
    public render = () => this.props.children;
}

export const OutsideClickHandler: React.ComponentClass<OnClickOutsideProps> = onClickOutside(XOutsideClickHandler);
export const Menu = () => {
    const [open, setOpen] = React.useState(false);
    return (
        <OutsideClickHandler onClickOutside={() => setOpen(false)}>
            <input type="button" onClick={() => setOpen(true)}/>
            {`menu is ${open}`}
        </OutsideClickHandler>
    );
};

If not, something like this should probably added to this repo.

Pomax commented 5 years ago

@gpietro @jmattheis can you both please file new issues, as they're different questions that warrant different discussions, neither of which should be taking place in a closed issue.

Pomax commented 5 years ago

(locked mostly to prevent more people from leaving comments on this closed issue, more than happy to discuss any and all things in their own, new issues, of course)