lookfirst / mui-rff

MUI 5 / Material UI + React Final Form
https://lookfirst.github.io/mui-rff/
MIT License
490 stars 93 forks source link

DatePicker: Selecting the date from popup, should mark the field as `touched`. #803

Open Dragomir-Ivanov opened 2 years ago

Dragomir-Ivanov commented 2 years ago

Hi there, I want to propose following usability feature.

Currently, when selecting the date from calendar popup, the RFF field is not marked as touched, which is not true. One needs to enter the TextField corresponding to that DatePicker, for the field to be marked as touched.

My proposal is to manually call restInput.onFocus(event); from the DatePicker onchange callback.

<MuiDatePicker
            onChange={(event) => {
                            onchange(event)
                            restInput.onFocus();
                        }}
            value={(value as any) === '' ? null : value}

Thank you.

lookfirst commented 2 years ago

@Dragomir-Ivanov Hey! Thanks for the report... I wonder if something else is going wrong here?

Dragomir-Ivanov commented 2 years ago

I think not. When clicking on the Calendar IconButton on the right side, the click event doesn't propagate to the TextInput field at all, so its onFocus is not called. And it seems RFF doesn't emulate touched when field is changed, which seems right thing to do, since one might change a field via the FormApi, so changing doesn't guarantee touching. But in our case, Changing the DatePicker value, guarantees a touch.

The same applies I presume for DateTimePicker as well.

lookfirst commented 2 years ago

I guess what I'm not understanding is why the onBlur/onFocus stuff isn't working right. Can you provide a codesandbox?

2022-07-21 at 17 57 52

Dragomir-Ivanov commented 2 years ago

Okay I will provide a codesandbox for you, today or tomorrow. In short, there is not interaction with this TextField you are highlighting. I just click on the Calendar IconButton, that is rendered at the right on this TextField by default, popup calendar appears, I click on the date, and value of DatePicker changes accordingly. There is no interaction with this TextField, I am not clicking on it, I am not focusing it, I am not typing anything in it, so it doesn't fire its onFocus/onBlur callbacks.

lookfirst commented 2 years ago

That almost seems like more of a bug in the DatePicker since it is the one rendering that icon and handling the button clicks on it. It should call the onFocus of the renderInput value.

Dragomir-Ivanov commented 2 years ago

Well DatePicker is a higher order component, so it doesn't have a notion of focused. The issue really arises when DatePicker is atomized and used within RFF, because RFF has a notion of touched fields. It doesn't care if it is <input> or some other input-like component.

lookfirst commented 2 years ago

Except they generate that icon and handle the clicks on that icon. If they aren't forwarding the events from those clicks, then there isn't much that we can do about that.

Dragomir-Ivanov commented 2 years ago

Yes, my proposal was a workaround that would work for RFF, not in general. In any case, thank you for your time. If you are not willing to implement that, I understand :)

lookfirst commented 2 years ago

Wait, I didn't say I didn't want to implement anything. =) I just don't think it is an issue in mui-rff, so we might be barking up the wrong tree here. Your proposed solution doesn't really make any sense since you're overriding the onChange method and I'd like to see a codesandbox that demonstrates the issue so that I can play with things and try to get a better understanding of what is wrong and where the issue is. Hope that makes sense.

Dragomir-Ivanov commented 2 years ago

Hi there! Here is a codesandbox demonstrating the issue. Observe the Console while you are changing the dates. Use only the button and the popup, no keyboard.

https://codesandbox.io/s/immutable-bush-xqfps9?file=/src/App.tsx

It turned out that RFF needs the two calls onFocus and onBlur in sequence in order to raise its touched flag. Thank you!

lookfirst commented 2 years ago

@Dragomir-Ivanov Very nice work. I'll look into this sometime this week (I'm going to be traveling soon). I can see that all the events here are getting very confusing and need to be thought out a bit more. There really shouldn't be a need to call those from onChange imho... something is not being passed through the property chain correctly.

Dragomir-Ivanov commented 2 years ago

Hi @lookfirst , I appreciate your engagement. No worries about timing, I have a workaround for now. Everything is passed correctly, however you are only passing OnFocus/OnBlur callbacks to the embeded TextField for the DatePicker. There are no OnFocus/OnBlur for the IconButton exposed, but for the form usage they are not needed.

The issue arises only for RFF trying to implement DatePicker, because DatePicker provides a value, either through its button, or the text field, but for the RFF this doesn't matter, since it doesn't need to know the details. Providing data via text field, marks the RFF as touched(calling onFocus/onBlur), but providing data via the button, does not. Ideally we would have access to the button events, but we don't. I don't see this as such big deal in order to poke the guys at MUI. Emulation via the onChange callback, is the best next thing, I think.

Give it a good though, no hurries.

Have a nice an save trip!