search

tableau / tableau-ui

React UI components that have the look-and-feel of Tableau.
https://tableau.github.io/tableau-ui/
Other
95 stars 28 forks source link

Use React.forwardRef on functional components #12

Closed tjallingt closed 5 years ago

tjallingt commented 5 years ago

https://reactjs.org/docs/forwarding-refs.html

React.forwardRef can be applied to "simple" styled components to give developers access to the underlying dom node. Probably useful on Button, Checkbox, DropdownSelect, Pill, Radio and TextField components.

craigkovatch commented 5 years ago

We forward props on these components so you should be able to just set ref in the usual manner. If I’m missing something, please provide a code sample that shows what is currently not-working so we can understand your need better :)

tjallingt commented 5 years ago

Admittedly i didn't test them all. I just tried to focus the TextField and it didn't work see this example: https://codesandbox.io/s/m74o6yykv9

AFIAK forwarding props doesn't work with refs since React swallows those (just like it swallows the key prop). refs on class components point to the class instance (which can or cannot be intended behaviour) but refs on function components just plain don't work without React.forwardRef.

Note: using react hooks for brevity, shouldn't change the functionality (but i didn't test it so who knows really)

craigkovatch commented 5 years ago

Ah, yes, you're right about key and ref getting swallowed, my mistake. We will switch to using forwardRef -- thank you for the excellent report and information!

craigkovatch commented 5 years ago

@tjallingt Version 2.1.0 has been published, could you please verify that refs work as you expect and let me know?

tjallingt commented 5 years ago

Great! Everything works as expected. Even <Spinner /> got a ref 😄