zillow / react-slider

Accessible, CSS agnostic, slider component for React.
https://zillow.github.io/react-slider
MIT License
892 stars 231 forks source link

Warning when using React 0.14 #51

Closed hugihlynsson closed 9 years ago

hugihlynsson commented 9 years ago

When using react-slider in a React 0.14 project, the following warning is issued by React:

ReactDOMComponent: Do not access .getDOMNode() of a DOM node; instead, use the node directly. This DOM node was rendered by ReactSlider

qwtel commented 9 years ago

Ah, thank you for trying the component with react 0.14.

I've heard of this change, but I don't know how to best handle it, so that the component can be used with react 0.13 and 0.14. Checking for getDOMNode to be defined seems like a pretty brute way to do it :/ Happy to receive any input concerning this.

hugihlynsson commented 9 years ago

Yeah, I'm not sure either what is the best way to go forward with this, without breaking backwards comparability. In React 0.14, this is only a warning, so maybe is should just stay like it is until it will be totally deprecated.

qwtel commented 9 years ago

Now would be the time to fix the warning so the component doesn't break when it is totally deprecated. I assume there will be a suggested way of handling this once 0.14 is released. Or we just add the check...

canfie1d commented 9 years ago

I think .findDOMNode() replaces it, no? http://www.omerwazir.com/posts/react-getdomnode-replaced-with-findDOMNode/

hugihlynsson commented 9 years ago

@canfie1d That change was introduced in React 0.13, but 0.14 includes further changes. findDOMNode() is no longer a part of the react package, but rather a new react-dom package. Also, refs that point to a DOM component now are the DOM node, and should not be wrapped with findDOMNode(). See more here in the React 0.14 blog post

canfie1d commented 9 years ago

Ah- I see. Thanks for that.

dmitry commented 9 years ago

Looks like the right way to release new version of a package with strict dependency to react 0.14.

PS. Similar idea: https://github.com/moroshko/react-autosuggest/commit/4ab699dc20114c2287e24ace3129da4ddfb4f04b

qwtel commented 9 years ago

Thanks for looking it up :+1:

martpie commented 9 years ago

Any news on that ? react 0.14 is out now.

Instead of findDOMNode, just use refs:

render() {
    return <div ref='element'></div>;
}

doSomething() {
    console.log(this.refs.element); // will print the node element to the console
}
nuclearspike commented 9 years ago

I too am looking forward to .14 support.

nuclearspike commented 9 years ago

The warning stops happening if you just remove the ".findDOMNode()" from 2 places in the code.

martpie commented 9 years ago

I submitted a PR for that, but the maintainer seems inactive.

gabssnake commented 9 years ago

@KeitIG your PR doesn’t seem to update package.json or handle compatibility with 0.13

martpie commented 9 years ago

True dat, doing it asap.

martpie commented 9 years ago

Done.

mauricesvay commented 9 years ago

:+1:

Martindelataille commented 9 years ago

Waiting fix too. @KeitIG Thanks

martpie commented 9 years ago

@Martindelataille, yep, would be great if the PR was accepted now :)

martpie commented 9 years ago

Issue can be closed now a new version is released. poke @hugihlynsson

hugihlynsson commented 9 years ago

Super!