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

Use ref instead of ReactDOM.findDOMNode() #64

Merged
merged 1 commit into from
May 14, 2017

Conversation

gwu
Copy link
Contributor

@gwu gwu commented Feb 28, 2017

ReactDOM.findDOMNode() is discouraged, so we use refs instead (https://facebook.github.io/react/docs/react-dom.html#finddomnode).
This allows us to use this in our server-side render correctly.

ReactDOM.findDOMNode() is discouraged, so we use refs instead.
This allows us to use this in our server-side render correctly
@JedWatson
Copy link
Owner

Really neat solution, thanks @gwu

@@ -126,6 +126,14 @@ var HammerComponent = React.createClass({
}
}, this);

var self = this;
props.ref = function(domElement) {

Choose a reason for hiding this comment

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

@JedWatson @gwu
cc @silentcloud
This PR will cause two problem:

  1. break change, see below use case, the children's string ref will be override by Hammer
        <Hammer
          direction="DIRECTION_HORIZONTAL"
          onPanStart={this.onPanStart}
          onPan={this.onPan}
          onPanEnd={this.onPanEnd}
        >
          <div className={`${prefixCls}-content`} ref="content">
            {children}
          </div>
        </Hammer>
  1. cause react warning: https://facebook.github.io/react/warnings/special-props.html

Choose a reason for hiding this comment

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

+1 Yes this is breaking ref for child components

Choose a reason for hiding this comment

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

#87

paranoidjk referenced this pull request in react-component/react-hammerjs May 16, 2017
moretti added a commit to marvelapp/react-hammerjs that referenced this pull request Feb 26, 2018
moretti added a commit to marvelapp/react-hammerjs that referenced this pull request Feb 26, 2018
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.

6 participants