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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make onChange event handler accept standard event object as argument #1860

Closed
smashercosmo opened this issue May 3, 2021 · 16 comments
Closed

Comments

@smashercosmo
Copy link

馃檵 Feature Request

Currently there is inconsistency in how react-area handles events. Almost all of the event handlers accept standard event object as argument. Except for onChange handler, which only accepts 'value'.

馃 Expected Behavior

onChange handler should accept standard event object as argument

馃槸 Current Behavior

onChange handler only accepts value as argument

馃拋 Possible Solution

Introduce onValueChange event which will pass value to the event handler and change onChange event to pass event object to the event handler (in equivalent to onFocus and onFocusChange events)

馃敠 Context

This inconsistency makes it hard to integrate with third-party libraries (like react-hook-form) which pass standard onChange handler to the component.

@ktabors
Copy link
Member

ktabors commented May 5, 2021

We opted to be consistent across all our onChange events rather than having some with value and others with events. Thank you for your feedback.

@valikron
Copy link

valikron commented May 9, 2021

@ktabors I think it would be nice to pass the event object at least as a second parameter. For example I'm now trying to user react-spectrum with react-hook-form and it expects an event object. So I run into this issue.

@devongovett
Copy link
Member

I think the problem is that there isn't always a DOM event for all change events. It might be true for input elements but for other components that's not the case. We'd rather be consistent and avoid leaking implementation details of our components if possible.

What is react-hook-form doing with these events? If it requires a change event, wouldn't that mean it won't work with anything other than native DOM elements? For example, a custom select dropdown isn't going to have native DOM change events available.

@valikron
Copy link

valikron commented May 10, 2021

@devongovett I'm a big fan of consistancy too. But I'm not quite sure that providing the change event would necessarily mean leaking implementation details. I think when you change the standard behaviour of a common element like input, it's a good practice to give at least some fallback. A lot of other ui libraries like material-ui und ant-design etc. also stick to the change event. I think it's simply because it's the default behaviour.

I don't know what exactely react-hook-form does with those events, but I'm assuming not much beside reading the value. But the register function expects a change event. What I'm actually trying to build is a more smart Form component, like in this example, because I don't quite like how react-spectrum handles the forms. It's ok for small forms but it's a bit boilerplatty. I don't want to repeat the validation logic or the logic needed to prevent a form from being submitted, if the form is prestine or invalid. I've found a workaroung by using the controller hook, but if react-spectrum would provide the change event, I could reduce some overhead.

@smashercosmo
Copy link
Author

@devongovett by introducing new onValueChange event you can achieve both consistency and interoperability. Use onValueChange everywhere and onChange only in situations when there is a native onChange event.

@devongovett
Copy link
Member

The problem is it won't work for all form components. Only text fields and maybe radios/checkboxes. For more complex custom form controls like custom selects, comboboxes, number fields, sliders, etc., there is no DOM change event available. So would those components just not work with react-hook-form? I feel like this is more of a limitation of react-hook-form only working with native DOM form elements and not custom components...

@snowystinger
Copy link
Member

I just took a look at the react-hook-form documentation. You noted this earlier as a workaround, but it's actually the recommended way to handle this as per react-hook-form documentation. Their recommendation is to use Controller when integrating with UI libraries
https://react-hook-form.com/get-started#IntegratingwithUIlibraries
which the API for this https://react-hook-form.com/api/usecontroller/controller specifies onChange: (value: any) => void

@valikron
Copy link

valikron commented May 13, 2021

@devongovett I don't propose it for all controls, just for those that have a change event. I think they should not hide it completetly and give some fallback option instead. Either as a second parameter or as a different property. Something like @smashercosmo proposed it to do. And not because it's not possible with react-hook-form (and alike) but because it changes the default behaviour that can become handy when you want to extend some functionality, as you can see in my example. Edit: And for other controls there is the mentioned Controller component in react-hook-form. But like I said, it creates some overhead in cases where it's not absolutely needed if you have the change event.

@snowystinger yes, thanks, you are right. I've tryed it once and it didn't worked out and I was irrititated by the name of the onChange parameter, that is still event. So i've striked my solution through. But now I've tryed it again and saw that my problem was related to something else. But now it's working fine.

@devongovett
Copy link
Member

devongovett commented May 27, 2021

I think since there is an alternative here, I'm going to close this. The main reasons we don't pass through DOM events in onChange (and in general across most of our events) are:

  1. Consistency. As discussed above, we'd prefer for all of our events to have consistent arguments.
  2. Leaking implementation details. If the implementation of a component changed in the future to use a different DOM structure or events internally, we don't want to expose this change to consumers which could break their code.
  3. Cross platform support. The API is designed to work across platforms, e.g. on React Native. If we expose DOM implementation details, the API will not work the same way across platforms, which makes code consuming our components less portable.

@alavkx
Copy link

alavkx commented Oct 5, 2022

I totally agree with your response given the design goals of the library. That said, it leaves a feature gap for every user wanting to doing any sort of form validation that implements their design system with react-aria.
I'm trying to implement a design system with behaviors from react-aria to be compatible with uncontrolled components via react-hook-form and I'm getting slaughtered (I'm not very smart 馃挙 ). It's really not a good fit. It leaves me wondering what abstractions Adobe use internally to solve these problems. Surely there is something internal.

Given that form validation is basically a requirement for any app of meaningful complexity鈥攕hould react-aria attempt to offer a solution? Currently, I'm given the option to either write my own form library, or stop using react-aria鈥攁nd neither of those options sound appetizing.

Forms are hard, and you likely don't want to take on a problem of this scope and I understand that. Do you think its appropriate for react-aria to attempt to solve this issue? Or maybe an adjacent project should be spun off to solve compatibility between react-aria and another form library in the ecosystem?

@devongovett
Copy link
Member

Did you try using the useController hook from react-hook-form? I believe we have some apps using that internally. https://react-hook-form.com/api/usecontroller

@alavkx
Copy link

alavkx commented Oct 5, 2022

If you're ok with tightly coupling your design system to your form library of choice, that's a good approach. Maybe I just need to make that admission and get on with it 馃槄

@devongovett
Copy link
Member

I'm not sure I'd include it in the actual design system components. React Spectrum definitely doesn't. Applications should be able to combine them themselves. For example:

 const {
    field: { onChange, value, ref },
   // ...
} = useController(/* ... */);

<MyDesignSystemTextFieldBuiltWithReactAria onChange={onChange} value={value} ref={ref} />

@alavkx
Copy link

alavkx commented Oct 5, 2022

Right that's the bit I'm finding particularly challenging. onChange here expects a DOM event, but event handlers in react-aria are platform agnostic. It seems I can make it work by using mergeProps in some scenarios, but the implementation is challenging for someone not so experienced with react-aria internals.
An abridged (omitting RadioGroup for brevity) implementation (that I still haven't gotten working) might look like

interface RadioProps extends Omit<AriaRadioProps, 'onChange' | 'onBlur'> {
  onChange?: React.ChangeEventHandler<HTMLInputElement>
  onBlur?: React.FocusEventHandler<HTMLInputElement>
}
const Radio = React.forwardProps(function Radio({ onChange, onBlur, ...props}, ref) {
  const state = React.useContext(RadioGroupContext) as RadioGroupState
  const { inputProps } = useRadio(
    props,
    state,
    useObjectRef(ref)
  )
  return <OtherStuff>
    <input {...mergeProps(inputProps, focusProps, { onChange, onBlur )} ref={ref} />
   </OtherStuff>
})

Anyways, I'm not expecting any type of support. More to say, is my struggle representative of others' struggles attempting to create a design system based on react-aria that plays nicely with form libraries? If so, is there some missing piece either in the API or the docs that could bridge the gap?

I find myself often referring to react-spectrum source code for these implementations and am left in a weird spot when some essential stitching is covered by a @react-spectrum/utils function like useDOMRef or useFocusable. Or with confusion鈥攈ow does this work?鈥攚hen I see that the forwarded ref is not the ref that is being attached to the input.

A scenario I'm trying to avoid in my own design system is consumers having to answer the question鈥攈ow do I connect this component to form state? Maybe Radio is not compatible with react-hook-form/register, but TextField is compatible, leading to this disparate ecosystem.

I appreciate your patience with my poorly constructed observations 馃ザ

@devongovett
Copy link
Member

Hmm, according to the useController docs, onChange wants a value, not an event.

Object Name Name Type Description
field onChange (value: any) => void A function which sends the input's value to the library.It should be assigned to the聽onChange聽prop of the input and value should not be聽undefined.

In general, I'm not sure there is a way to avoid useController. Even if we provided a DOM event for say textfields, we couldn't for custom selects, date pickers, etc. So seems better to me to just be consistent about it.

@alavkx
Copy link

alavkx commented Oct 5, 2022

Hmm, according to the useController docs, onChange wants a value, not an event.

My mistake, I shouldn't have assumed that ReturnType<typeof register>> and ReturnType<typeof useController["field"]> were similar. Thanks for the correction.

This conversation is sorta leading me to believe that building a design system with a DOM-friendly, uncontrolled-field-friendly API is more to blame for the complexity than react-aria.
Maybe what I'm actually looking for is an alternative to react-hook-form/register that is more compatible with platform agnostic event handlers.

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

No branches or pull requests

6 participants