-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[TextField] Can't pass a ref to TextField component #1083
Comments
Is there any reason that wrapping the input in a |
If you can't use Dan's suggestion I'm in favour of option one. It's the "react" way of doing things and using a ref only isn't an option anymore because of "wrapper hell" |
Thanks, placing the |
What if one wanted to use https://github.com/text-mask/text-mask/tree/master/react#customize-rendered-input-component with a |
@raunofreiberg I'm not familiar with |
The Typing in the input helps you just fill in the blanks for the date with a nice UX. |
Yup, agreed that is a nice UX, and not achievable in the current implementation. This may even be something worth supporting natively in the component. @AndrewMusgrave what would it look like to implement ref forwarding with WithRef HOC? Are there other components where access to the underlying DOM node might be useful? |
it's also not possible to use https://react-hook-form.com/ because it requires to register inputs via please also see #1918 |
@sijad Do you in anyhow found any way to use react-hook-form? If not, what form validation library would you recommend to use? |
@andychong1996 unfortunatly no. I'm using Formik v2 + Fonk in my project. |
Reaching into components is something that Polaris doesn't recommend since it's likely that a future upgrade will eventually break something. If your deadset on using react hook form, you can still access the input element using web api's. If you need to register the input element multiple times, you may want to look into using a mutation observer as well. import React, { useCallback, useState, useEffect } from "react";
import { TextField } from "@shopify/polaris";
function Input({ children, id }) {
const [inputNode, setInputNode] = useState();
useEffect(() => {
setInputNode(document.querySelector('input'));
}, [id]);
return children(inputNode);
}
export default function TextFieldExample() {
const [value, setValue] = useState("Jaded Pixel");
const id = "my-text-field";
const handleChange = useCallback(newValue => setValue(newValue), []);
return (
<Input>
{inputNode => {
console.log(inputNode);
return (
<TextField id={id} label="Store name" value={value} onChange={handleChange} />
);
}}
</Input>
);
} With that being said, we don't recommend reaching into components 😄 |
Material UI supports ref. Are there any changes Polaris plan? |
It seems like the overall "don't reach into components" design principle is breaking compatibility with the most popular React form state management library. In this case, is it really adding that much future compatibility guarantees? TextField will always have to wrap some user-facing input element, and should always be a good citizen and emit browser-land focus and blur events. Those two bits are what I also might call this DX hostile -- this decision for "a future upgrade that may break something" is causing real pain for devs now. Maybe a better option would be to expose the ref, and reserve the right to change which element exactly is referenced in the future, enabling us forced-to-use-your-thing consumers to still play nice with the rest of the ecosystem while bearing responsibility for the maintenance burden if we reach in too far? Knowing that there is a (super poorly performing) work around using the DOM that you folks documented too -- why make folks have to do the workaround? |
Here is my workaround: import { TextField } from '@shopify/polaris'
import { type ForwardedRef, forwardRef, useId, useImperativeHandle } from 'react'
import { type InputTextProps } from './@types'
function InputText({...props}: InputTextProps, ref: ForwardedRef<HTMLInputElement>) {
const id = useId()
useImperativeHandle(ref, () => document.getElementById(id))
return (
<TextField
{...props}
id={id}
/>
)
}
export default forwardRef<HTMLInputElement, InputTextProps>(InputText) |
This works: import { Controller, useForm } from "react-hook-form";
type Inputs = {
name: string;
};
export default function MyField() {
const { control } = useForm<Inputs>();
return(
<Controller
name="name"
control={control}
defaultValue=""
rules={{ required: "Name is required" }}
render={({ field, fieldState: { error } }) => (
<TextField
label="Name"
name={field.name}
value={field.value}
onChange={(value) => field.onChange(value)}
onBlur={field.onBlur}
error={error && error.message}
autoComplete="off"
/>
)}
/>
);
} |
While workarounds are possible, please consider re-opening this issue and exposing the refs, this is a standard react pattern that the most popular ui libraries follow, why is Polaris unique here? most importantly dealing with user-interactive elements such as form elements. |
Background
We have a use case where we have two TextFields and their heights are being matched. The current way we're accomplishing this is by using a ref to get the height of the TextFields from the
DummyInput
and passing back the larger of the two.Problem
With the
3.6.0
version, TextField is now being wrapped withwithAppProvider
meaning that the ref is no longer being set on the component and it is breaking our section since we can't doref.current.input
to get the height.Solution
I have two solutions in mind and have seen them used in the current code base:
Use the
withRef
higher-order component withcompose
in TextField so that refs can be forwarded to the component. Keeping the functionality of our use case.Add a
data-height
attribute on the component that will give the user the height that the component calculates so that we don't have to use a ref.I'm leaning towards solution 2 because, at least for our use case, we just need the calculated height of the textfields. The use case with using a ref would probably be where someone needs to modify the element and I'm not sure that's something we would want to let happen.
The text was updated successfully, but these errors were encountered: