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

Allow usage of ref on custom components #273

Closed
clayne11 opened this issue Sep 18, 2018 · 14 comments · Fixed by #293
Closed

Allow usage of ref on custom components #273

clayne11 opened this issue Sep 18, 2018 · 14 comments · Fixed by #293

Comments

@clayne11
Copy link

clayne11 commented Sep 18, 2018

React 16.3 introduced the concept of forwarding refs as a first-class citizen. We can remove the usage of innerRef and just use the regular ref prop. We can make this backwards compatible by using innerRef if it exists however we shouldn't throw if a custom component doesn't have innerRef - ref can work just fine.

This library currently breaks with styled-components@4 due to this issue.

@clayne11 clayne11 changed the title Remove innerRef Allow usage of ref on custom components Sep 18, 2018
@clayne11
Copy link
Author

Looks like we can't actually make this backwards compatible. I think we should release a new major version that no longer uses innerRef.

@clayne11
Copy link
Author

@brigade-slack I'm happy to make a PR for this if you agree.

@trotzig
Copy link
Collaborator

trotzig commented Sep 20, 2018

I’m in favor of the breaking change. Perhaps coupled with a note in the readme about needing to use an older version for innerRef. Thanks for helping out!

@clayne11
Copy link
Author

@trotzig I'm also having quite a bit of frustration with the test suite. How do you feel about updating to Jest and Enzyme? The current test suite is quite crappy to use.

@trotzig
Copy link
Collaborator

trotzig commented Sep 20, 2018

That sounds like a good idea in general. I remember setting this up years ago when neither of those were a thing. The only thing I would worry about is that the code relies on getting proper measurements from the browser. I’m not sure jsdom will provide those unless we mock things (which might be okay).

@clayne11
Copy link
Author

OK I'll have a look at updating the tests in a separate PR and see how it goes.

@piuccio
Copy link

piuccio commented Nov 5, 2018

Any progress on this? I'm trying to use this library with styled-components@4 but it won't work

@clayne11
Copy link
Author

clayne11 commented Nov 5, 2018

It was a pain getting the tests working and I ran out of time working on it. I forked it and fixed it at @clayne/react-waypoint but the tests are failing so I never submitted a PR.

My company Casalova is using it in production and everything works.

@trotzig
Copy link
Collaborator

trotzig commented Nov 18, 2018

Is #278 addressing this concern perhaps?

@Andarist
Copy link
Contributor

@trotzig yeah, my PR is fixing this problem. With my PR ref prop can be used when wrapping DOM elements or elements wrapped with React.forwardRef (such as styled-components@4). Naturally for other elements you still have to use innerRef.

@twxia
Copy link

twxia commented Jan 22, 2019

Hi @Andarist but for your change, this line of code, when styled-component pass to isDOMElement is false so it can't get inside that condition.

I need to write it as below code to let the waypoint works fine again:

const WrapperWithRef = ({ innerRef, ...props }) =>
 <Wrapper ref={innerRef} {...props} />);

@Andarist
Copy link
Contributor

@twxia Which version of styled-components do you use?

@twxia
Copy link

twxia commented Jan 25, 2019

@Andarist styled-components@latest (4.1.3) I think all of v4 can't work well in that change

@Andarist
Copy link
Contributor

Somehow I've made a mistake and was calling isForwardRef with wrong argument, I've prepared a PR with a fix for this just now.

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 a pull request may close this issue.

5 participants