Shopify / polaris

Shopify’s design system to help us work together to build a great experience for all of our merchants.
https://polaris.shopify.com
Other
5.81k stars 1.17k forks source link

[TextField] Can't pass a ref to TextField component #1083

Closed theodoretan closed 5 years ago

theodoretan commented 5 years ago

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 with withAppProvider meaning that the ref is no longer being set on the component and it is breaking our section since we can't do ref.current.input to get the height.

Solution

I have two solutions in mind and have seen them used in the current code base:

  1. Use the withRef higher-order component with compose in TextField so that refs can be forwarded to the component. Keeping the functionality of our use case.

  2. 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.

danrosenthal commented 5 years ago

Is there any reason that wrapping the input in a div, placing the ref on that div, and getting the height from that would not work?

AndrewMusgrave commented 5 years ago

Is there any reason that wrapping the input in a div, placing the ref on that div, and getting the height from that would not work?

clientHeight doesn't include margin or border so that may be a viable option.

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"

theodoretan commented 5 years ago

Thanks, placing the ref of the div works 👍

raunofreiberg commented 5 years ago

What if one wanted to use https://github.com/text-mask/text-mask/tree/master/react#customize-rendered-input-component with a TextField from Polaris? The library needs access to the input DOM node to manage masking.

danrosenthal commented 5 years ago

@raunofreiberg I'm not familiar with text-mask, but on a quick glance it appears to be an abstraction on top of the pattern property. We expose a pattern prop on TextField, which translates directly to the pattern attribute on the input element. Is there any reason that does not fit your use-case?

raunofreiberg commented 5 years ago

The pattern attribute doesn't seem to mask the input. What I exactly have in mind is the date input here: https://text-mask.github.io/text-mask/

Typing in the input helps you just fill in the blanks for the date with a nice UX.

danrosenthal commented 5 years ago

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?

sijad commented 5 years ago

it's also not possible to use https://react-hook-form.com/ because it requires to register inputs via ref prop.

please also see #1918

andychongyz commented 4 years ago

@sijad Do you in anyhow found any way to use react-hook-form? If not, what form validation library would you recommend to use?

sijad commented 4 years ago

@andychong1996 unfortunatly no. I'm using Formik v2 + Fonk in my project. yup is more popular validator but Fonk was simpler. for formik you can also use this projec: t: https://github.com/SatelCreative/formik-polaris

AndrewMusgrave commented 4 years ago

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 😄

sonatard commented 3 years ago

Material UI supports ref. https://github.com/mui-org/material-ui/pull/23174

Are there any changes Polaris plan?

airhorns commented 1 year ago

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 react-hook-form wants to use a ref to couple to, and that seems pretty reasonable as both of those are a fundamental part of any text field's contract. Exposing a ref in the long term that commits to that (and only that) doesn't extend or change the contract if it preserves just those elements.

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?

SearheiParkhamchuk commented 10 months ago

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)
MTraveller commented 3 months ago

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"
        />
      )}
    />
  );
}