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

Text component doesn't correctly attach ref passed via props #6320

Closed
maclockard opened this issue Aug 16, 2023 · 4 comments · Fixed by #6331
Closed

Text component doesn't correctly attach ref passed via props #6320

maclockard opened this issue Aug 16, 2023 · 4 comments · Fixed by #6331

Comments

@maclockard
Copy link
Contributor

The Text component allows for a ref for the underlying HTML element via including React.HTMLAttributes<HTMLElement> in the props. However, the component passes its own ref in here. This results in a ref passed to Text never being invoked/set.

Ideally instead this ref should compose with whatever ref is passed

@pgoldberg
Copy link
Contributor

Hey @maclockard!

I may be wrong, but I actually don't think ref is exposed in React.HTMLAttributes<HTMLElement> (it would be in React.DetailedHTMLProps<React.HTMLAttributes<HTMLElement>>). Would you want this as a feature request?

@adidahiya
Copy link
Contributor

I may be wrong, but I actually don't think ref is exposed in React.HTMLAttributes<HTMLElement> (it would be in React.DetailedHTMLProps<React.HTMLAttributes<HTMLElement>>). Would you want this as a feature request?

Yeah, that sounds right. ref is not currently a supported prop of <Text>. Open to adding this prop as a feature request.

image

@maclockard
Copy link
Contributor Author

Ahhh. Looking a bit closer at our types I think its from us being on an older version of blueprint that still uses class components, which the React types auto adds a ref prop to looking at it?

Yeah, would be great to have this as a feature request!

@adidahiya
Copy link
Contributor

React types auto adds a ref prop

With the class component implementation, you get a ref to the Text component instance, not the DOM node. Part of the motivation for migrating to FCs is to use forwardRef so that ref gives you access to the DOM node, like it would with intrinsic elements.

I'll take a look at your PR.

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

Successfully merging a pull request may close this issue.

3 participants