-
Notifications
You must be signed in to change notification settings - Fork 64
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
chore: refactored from findDOMNode to refs #476
chore: refactored from findDOMNode to refs #476
Conversation
Styles
Contributors |
FYI @sherwinski @jayeb this is technically a breaking change since this would break support for React <16.3 (this library currently supports React 16.0, 16.1, and 16.2). If support for these versions is desired, then callback refs can be used source. |
Good point. I can refactor towards a callback ref if that'd be preferable. |
Thanks for the reply @tstirrat15. @sherwinski and @jayeb will reply here soon with a strategy 👍. Thanks for the PR btw 🙌 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the hold up @tstirrat15, and thanks for looking into this! I am going to err on the side of providing wider support and say that we should use callback refs here.
Done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thank you 🙌
I'm seeing some style changes when I run npm run pretty
locally. Usually Prettier would auto fix this but it hasn't been working for some time it seems. I'm going to go ahead and commit those changes on top of what you already have and then this is good to go!
👏 👏 👏 |
Fixes #475
Description
findDOMNode
is deprecated inStrictMode
. It's recommended against in the official docs, and if the functionality that it enables is required,ref
s are a good way of addressing the problem.This PR refactors away from
findDOMNode
towardsref
s.Changes
react-imgix
components to useref
s in favor offindDOMNode
mount
instead ofshallow
, sinceshallow
doesn't create or useref
sSteps to Test
See that the refactored tests are sane and passing.
Note
I wasn't quite sure what sort of work this represented (fix/feature/chore). Please let me know if a different designation is more appropriate.