search

Shopify / hydrogen

Hydrogen lets you build faster headless storefronts in less time, on Shopify.
https://hydrogen.shop
MIT License
1.27k stars 247 forks source link

Feature - Use `forwardRef` on primitive components that return HTML #625

Open DavidWittness opened 1 year ago

DavidWittness commented 1 year ago

Feature request brought up by discussion around best practices with components that return HTML.

Context

Original DevDocs issue

Slack convo

Goal

We can't anticipate what our devs will need to do with our components. One way to provide an "escape hatch" from the React lifecycle. This allows for vanilla JS changes where necessary (even when not ideal).

One example may be to have custom focus controls based on a specific UI flow that is inherently not declarative. Good example near the bottom of this LogRocket article

If a user can access the DOM element, they will have access to native browser APIs and can use vanilla JS libraries.

Developer Experience

When documenting these components, make specific note of where ref can be used. If a component returns more than one HTML element, specify which element is "forwarded".

andershagbard commented 8 months ago

This would be a nice feature to add to the components.

A component like Video feels unusable as you have to use native controls to interact with the video.

joelpierre commented 1 month ago

+1 this is an unusable component... Can we contribute to this or is it locked to only devs from Shopify? @andershagbard

andershagbard commented 1 month ago

+1 this is an unusable component... Can we contribute to this or is it locked to only devs from Shopify? @andershagbard

I'm not a Shopify employee, just a contributor. I've done a few PRs on this and had them merged, so everyone can help :)

blittle commented 1 month ago

@andershagbard which components are lacking in particular right now? We definitely welcome PRs.

joelpierre commented 1 month ago

Amazing @andershagbard & @blittle. Thanks for the responses.

@blittle I would say pretty much all lower level components that ultimately render single HTML elements like this would benefit from a forwardRef prop. That being said... React 19 is removing the need for it 😅 not sure when you would be supporting R19 but yeah... I guess this is a bit of a "just wait" sitch?

blittle commented 1 month ago

Agreed. We hope to get to React 19 sooner rather than later.

joelpierre commented 1 month ago

Screenshotted no take backsies 🤣 ... 😝

Looking forward to it. In the meantime I will avoid these lower level comps.

andershagbard commented 1 month ago

@andershagbard which components are lacking in particular right now? We definitely welcome PRs.

Most of them stilling missing forwardRefs. I did it for video components, as I needed them myself in a project. I would imagine all components could benefit from forwardRef.

It's rather simple to update the components with full backward compatibility.