Pomax / react-onclickoutside

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

StrictMode Compatibility #319

Closed vinnymac closed 5 years ago

vinnymac commented 5 years ago

Summary

This will prevent the need for always using findDOMNode and give users the ability to avoid warnings who rely on StrictMode.

This PR was made to fix #315

I am not happy with the name setClickOutsideRef but decided I would make the PR and get visibility rather than churning on bad names alone. I kept the solution similar to what we do for handleClickOutside, since it seemed like a similar situation where React is not exposing an API for us to rely on here in StrictMode.

The intention here is to support both Class based components and Function based components.

Classe Components

class Example extends React.Component {
  container = React.createRef()

  render() {
    return <div ref={this.container} />
  }

  handleClickOutside = (event) => console.log('outside', event)

  setClickOutsideRef = () => this.container.current
}

export default onClickOutside(Example)

Function Components

function Example() {
  const container = useRef()
  Example.handleClickOutside = (event) => console.log('outside', event) 
  Example.setClickOutsideRef = () => container.current

  return <div ref={container} />
}

export default onClickOutside(Example, {
  handleClickOutside: () => Example.handleClickOutside,
  setClickOutsideRef: () => Example.setClickOutsideRef,
})

TODOs

Perhaps in the future if this RFC comes to fruition we can replace this logic with a Fragment ref instead, and get access through that API.

I am not sure how many people actually need this, or care about warnings in StrictMode, this was the only warning in my app so I threw this together, if you aren't interested in this change I could understand it as it does increase the complexity of this lib. Users will have to do more for the same outcome.

alexmcmillan commented 5 years ago

this was the only warning in my app

Likewise.

Are there any plans to merge/release this? The big red console text makes me shiver with anxiety... ;)

Pomax commented 5 years ago

As for naming: naming variables based on what they are, even if that's an "ugly" name, is infinitely more useful to future (or current =) devs than some clever name that makes people go "wait what was this for?", so: this is a great name, it is the ref, for setclickootside. Its name is setClickOutsideRef. Let's keep it =)

Pomax commented 5 years ago

v6.9.0 published

bildja commented 5 years ago

@Pomax https://www.npmjs.com/package/react-onclickoutside this says the last version is 6.8.0

Pomax commented 5 years ago

hmm

Pomax commented 5 years ago

publish got blocked because the prepublish check relied on unix-only bash commands. Since all it did was check whether npm was >4, which is very much the case on any LTS node.js, I removed it and redid the publish. 6.9.0 should now be on npm.