[pickers] Pass event as the first argument to the onChange callback

[croraf]

• [ x] I have searched the issues of this repository and believe that this is not a duplicate.

Summary 💡

I think onChange callback from the pickers should be passed the event, same as what happens with other inputs. Or an object that extends the event.

Examples 🌈

https://codesandbox.io/s/date-picker-ks7fl?file=/src/App.js You can see it requires a second "handleChange" function.

Motivation 🔦

Motivation is to be able to have the same setValues handling as for other inputs. It will also expose other standard stuff, as with other native inputs.

Currently it is required to add second onChange handler next to the input text ones.

[eps1lon]

Could you elaborate a bit on the motivation? What do you mean with "setValues handling"?

Currently it is required to add second onChange handler next to the input text ones.

Could you provide a codesandbox that illustrates what you're currently doing?

[croraf]

(edited original post above - examples section)

[eps1lon]

It's not quite clear how the event would be used. It looks like you want to read event.target.value?

[croraf]

Yes. The same as regular text and select inputs. (checkboxes unfortunately work differently, but this is cause of HTML)

[oliviertassinari]

While we have facilitated the usage of event.target.value in a few components (Select + Slider), it doesn't look like something we should generalize lightly. 👍 for not doing it now (but waiting for a clear signal from the community that it's a pain with the pickers).

However, I personally think that developers might come up with valid ones if we expose the event. Hence why I would 👍 the idea to always expose the event.

[eps1lon]

Yes. The same as regular text and select inputs. (checkboxes unfortunately work differently, but this is cause of HTML)

But how is that useful for tht datepicker? I don't think you want to work with stringified values. Especially if you're using another date library like moment or dayjs.

[croraf]

Hmmm. I was accounting that event.target.value would be made of the same type as the first argument is now.

But now when you say, I guess event.target.value should be the same value that is propagated from the native input which is a string?

[eps1lon]

But now when you say, I guess event.target.value should be the same value that is propagated from the native input which is a string?

Exactly. And it's not even clear that the onChange event would always be triggered by user input into the textbox. Might be due to clicking a value in a picker.

That's why I'm asking what you need the event for. If you just need the value then you already have the value available.

[croraf]

I just wanted to have consistent API between TextFields, Select and DatePicker so that development might be slightly cleaner (by having only one handler function).

The root problem is then that HTML's onChange gets the event object as the first argument, instead of the value directly, which seems strange. So I now think they cannot be made consistent ever.

[eps1lon]

I just wanted to have consistent API between TextFields, Select and DatePicker so that development might be slightly cleaner (by having only one handler function).

But it would neither be the same type nor behave the same. For textfields you have a react change event that is associated with a textbox. The value would be associated with the target. Neither of these facts apply to the date(time) pickers.

So I now think they cannot be made consistent ever.

They are just not the same. That's why they shouldn't be "consistent".

[oliviertassinari]

@croraf Are you using a specific form library you struggle to integrate with?

[croraf]

@croraf Are you using a specific form library you struggle to integrate with?

No. I'm just annoyed having to write a special onChange handler for every form I use date picker in. Same with the checkbox. So if I have TextFields (text, multine, select), Checkboxes and DatePickers I have to use 3 onChange handlers. It is kind of annoying and messy. In theory I think these can all be the same, if the onChange would always receive value as the first argument for each of these form components. Then they would be all handled by for example:

const handleChange = (field) => (value) => {
    setValues({ ...values, [field]: value });
};

But because this is related to the native HTML onChange functionality, seems like there is nothing to be done to reconcile this.

---

Probably what would best describe my idea is to have onChange handler from TextField (text, multiline and select) and Checkbox have the signature onChange(value, event), different than the native DOM onChange handler.

[oliviertassinari]

@croraf Ok thanks for invalidating my assumption. For the checkboxes, it's the DOM API: event.target.checked 🤷‍♂️.

[croraf]

Ok thanks for invalidating my assumption. For the checkboxes, it's the DOM API: event.target.checked.

Sure.

I'll close the ticket.

The following would be a pretty significant intervention, but what do you guys think of having the onChange handler from TextField (text, multiline and select) and Checkbox have the signature onChange(value, event), different than the native DOM onChange handler.

[oliviertassinari]

but what do you guys think of having the

@croraf I think that we already had this discussion in the issue history around 2016, the current answer is no. We aim to be as close as possible to the native components. The less developers have to learn about Material-UI, the better. And yes, it can mean developers have to learn more about the DOM and React API.

I have added the "waiting for upvotes" label because I still think that this issue would be valuable for consistency with the other components that expose the event as the first argument, no matter what. In the data grid, we expose the event too, e.g. for https://next.material-ui.com/components/autocomplete/#events.

[croraf]

I understand the philosophy of aligning with native components, but I'm not sure this is the best way. Native HTML stuff is often buggy and drawn back by the legacy stuff.

[croraf]

With every new day, and form I make, I'm thinking more and more that for all form field types the first argument should be the newValue, and the second argument (which will be ignored most of the time) to be the event.

Not only this would allow to align all the form fields, which would require only one changeHandler function.
But sometimes it happens that I want to manually call the changeHandler, which requires me to create the event object with its structure. (For example I tried to set some form fields manually from the useEffect.)

I know this will not be aligned to how HTML native elements behave, but HTML native elements are awkward.

[matrixik]

There is already event.target.valueAsNumber so it would be nice to have event.target.valueAsDate or event.target.date together with event.target.name.

Previous requests (short search): mui/material-ui-pickers#182, mui/material-ui-pickers#473, mui/material-ui-pickers#483, mui/material-ui-pickers#644, mui/material-ui-pickers#974, mui/material-ui-pickers#1407, mui/material-ui-pickers#1728

[oliviertassinari]

@flaviendelangle Was this change considered as part of #3287?

For context, I landed here while I was working on using the date picker in Toolpad, I got tricked by the API. I was expecting the value to be the second argument of onChange, not the first one.

[flaviendelangle]

I don't think we will have the bandwidth for that change in v6 no

[LukasTy]

We've discussed that at its current state, we don't feel that making such a big breaking change and adding the event as the first argument would be the best course of action.
We've agreed that exploring the option of providing the original event in the second (context) argument could be a cleaner option. It would avoid a breaking change and massive headache if we wouldn't be able to provide the original event in certain change cases.
Currently, the context might only have a validationError, but after the suggested change it would also include an event:

export interface FieldChangeHandlerContext<TError> {
  validationError: TError;
+ event: React.ChangeEvent<HTMLInputElement>;
}

What do you think @oliviertassinari @croraf?