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

Expose refs for all components #126

Open
dcocchia opened this issue Mar 14, 2018 · 5 comments
Open

Expose refs for all components #126

dcocchia opened this issue Mar 14, 2018 · 5 comments
Assignees

Comments

@dcocchia
Copy link
Member

Many of our components do not set a ref on their main output, so users of the library will be forced to use findDOMNode to do things like focus on a button or input.

We should add a ref to each component to help with this.

@dcocchia
Copy link
Member Author

I think a nice way to do this would be to use the new ref forwarding api, use the same ref name for all components, and add tests to ensure we don't break that contract/api. It would require latest React, but maybe we could add in a fallback if the forwarding method isn't supported?

@tmarshall
Copy link
Contributor

@dcocchia sounds perfect. That's why I haven't started it, actually. Been hesitant to add in logic that may become moot once forwarded refs are in place.

@mmarcuccio mmarcuccio self-assigned this Oct 17, 2018
@mmarcuccio mmarcuccio added the WIP label Oct 17, 2018
@mmarcuccio
Copy link
Collaborator

mmarcuccio commented Oct 20, 2018

Any of the components using Radium will need to take this issue into consideration.
FormidableLabs/radium#1010

Ideally the Radium and withTheme higher order components would forward refs. This would maximize our code reusability and reduce complexity.

  • withTheme (supports ref forwarding)
    • Radium (supports ref forwarding)
      • Snacks Component (supports ref forwarding)

However, due to this issue we need to use React.forwardRef on the final export instead and pass the ref down the tree via a different property name (such as forwardRef). At the leaf level we can then assign the component's ref property to the value from props.forwardRef.

  • React.forwardRef
    • withTheme
      • Radium
        • Snacks Component ref={props.forwardRef}

@mmarcuccio
Copy link
Collaborator

I am putting my work on hold while the Radium team is investigating. If Radium ends up not supporting forward refs, we should discuss using the innerRef strategy that many other component libraries use.

@mmarcuccio
Copy link
Collaborator

mmarcuccio commented Dec 6, 2018

Forwardref is not going to be compatible with Radium. Let's move forward with the innerRef strategy.

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

No branches or pull requests

3 participants