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

Use forwardRef for all components #548

Closed
NicholasBoll opened this issue Apr 6, 2020 · 3 comments
Closed

Use forwardRef for all components #548

NicholasBoll opened this issue Apr 6, 2020 · 3 comments
Labels
breaking-change A change that will break something for consumers

Comments

@NicholasBoll
Copy link
Member

NicholasBoll commented Apr 6, 2020

🚀 Feature Proposal

We use an API that allows the spread of extra attributes and properties to a "container" element of a component via elemProps. In some cases we even explicitly forward a reference with a naming convention like inputRef. elemProps does not forward refs for us.

Assuming ColorInput is a class component:

<ColorInput ref={ref} inputRef={inputRef} />

ref; // reference to the instance of ColorInput
inputRef; // reference to the <input> element

React has a forwardRef API that allows us to formalize the use of ref as a property on all components. We would then apply that forwarded ref to the same containing element that extra props are applied to if it makes sense.

forwardRef allows us to keep the implementation of ColorInput private and allow us to give access to the public interface of the element.

Motivation

There are many instances where it is desired to have a reference to an element of a component. We already do this in some cases (e.g. inputRef for inputs). ref can be used on components now, but that just encourages the use of methods or properties of the component. We mostly treat that interface as private and could introduce problems later as we refactor code.

Without using forwardRef, the implementation details of a component become public. For example, changing ColorInput from a Class Component to a Functional Component becomes a breaking API change.

You can always get access to any element on the page using document.querySelector. This change would make it easier to get access to the underlying element of each component without resorting to tricks or hacks to do so.

Example

// start date
<DateInput ref={startDateRef} />
<DateInput ref={endDateRef} />

// Perhaps when we choose a value for the start date, we want to focus on the end date
endDateRef.current.focus()

Downsides

Another decision for Components - what element should be forwarded? Is it the same element as the element we spread extra elemProps over?

@NicholasBoll NicholasBoll added enhancement New feature or request question Further information is requested labels Apr 6, 2020
@anicholls anicholls added the breaking-change A change that will break something for consumers label Apr 6, 2020
@jpante jpante added the p:2 label Apr 13, 2020
@jpante
Copy link
Member

jpante commented May 12, 2020

Will need to look at this as a bigger milestone across all components.

@jpante jpante added the 5.x label May 26, 2020
@jpante jpante removed the question Further information is requested label Jul 20, 2020
@alexandrzavalii
Copy link
Contributor

just faced similar issue with TextButton.
TextButton does not forwardRef.

@jpante jpante removed the 5.x label Apr 13, 2021
@NicholasBoll
Copy link
Member Author

All inputs and buttons forward as do new components. The Svg component still does not forward the ref.

@jaclynjessup jaclynjessup moved this to ✅ Done in Canvas Kit Aug 2, 2022
@jaclynjessup jaclynjessup removed p:2 enhancement New feature or request labels Aug 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change A change that will break something for consumers
Projects
Archived in project
Development

No branches or pull requests

5 participants