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

StrictMode Compatibility #319

Merged
merged 1 commit into from
Aug 11, 2019
Merged

Conversation

vinnymac
Copy link
Contributor

@vinnymac vinnymac commented Apr 16, 2019

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

  • Supports Class Components
  • Supports Function Components
  • Include Tests
  • Add Documentation

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.

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

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... ;)

Copy link
Owner

@Pomax Pomax left a comment

Choose a reason for hiding this comment

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

This seems imminently sensible

@Pomax
Copy link
Owner

Pomax commented Aug 11, 2019

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 Pomax merged commit 756d87a into Pomax:master Aug 11, 2019
@Pomax
Copy link
Owner

Pomax commented Aug 11, 2019

v6.9.0 published

@bildja
Copy link

bildja commented Aug 19, 2019

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

@Pomax
Copy link
Owner

Pomax commented Aug 19, 2019

hmm

@Pomax
Copy link
Owner

Pomax commented Aug 19, 2019

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.

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

Successfully merging this pull request may close these issues.

The use of findDOMNode is deprecated in React Strict Mode
4 participants