Material-ui-pickers v4 - Githubissues

aaronhayes commented 4 years ago

Is your feature request related to a problem? Please describe. Material UI Pickers is getting close to releasing v4. This introduces some breaking changes to mui-rff. The first one being:

There is no more Keyboard components exported (like KeyboardDatePicker). From now any picker will allow keyboard input. For mobile, it is possible using the "pen" icon inside the dialog.

Describe the solution you'd like I believe the best way forward would be to remove support for KeyboardDatePickers and address any other breaking changes. As these would be breaking changes for mui-rff we would need to release v3.0.0.

lookfirst commented 4 years ago

Hi @aaronhayes ... thanks for filing... Yes, we will definitely need a v2.0 for this.

DASPRiD commented 4 years ago

Well, we are at 2.0 now ;)

lookfirst commented 4 years ago

Ha! Yes, we are... but that was for another reason! This one will need a mui-rff 3.0 now! =)

oliviertassinari commented 4 years ago

@lookfirst An update on the next version of the Date Picker, we are still far from a stable release. More changes are coming. Right now, we are working on the validation logic. Have you guys found a way to integrate with the built-in validation? We are looking for giving users more control over it. cc @dmtrKovalenko.

lookfirst commented 4 years ago

Hi @oliviertassinari, nice to see you here, see... this project got a tiny bit popular! =)

Not sure I understand your meaning. What validation logic and what built-in validation? Not sure what you're referring to here. Can you be a bit more specific?

Thinking aloud, I imagine that date pickers have two (or more) sets of validation. One is for the date picker itself (such as 'Is the date in a valid range') and another which is the output of the actual date that is picked.

For the validation within the date picker, I would suggest that just be a function which can be passed in (with maybe a sensible default function provided). For the validation of the output, that is handled through the RFF form library.

Another point for you, the thing I'm dealing with in this library is that people have different opinions on when to apply validation (open #186 and #191). Do we validate for onBlur, onChange, etc. (sadly, material spec isn't very clear / decisive on this topic).

The solution for this in my head is similar to what I'm saying above... provide my idea of a sane default and then externalize in the API the validation function so that people can do whatever they want. I just haven't had time to do this yet.

Finally... I'd love to switch to the new date pickers once things stabilize a bit. I don't have the time/energy to maintain two versions of this project just for date pickers, so that is why I'm still not there yet. Once I cut the next major release to support that, I'd like to put the prior (current) one on (low) maintenance mode.

Hope you're well and safe in these trying times.

oliviertassinari commented 4 years ago

this project got a tiny bit popular! =)

@lookfirst It's awesome to see that, forms are important :D.

Not sure I understand your meaning. What validation logic and what built-in validation? Not sure what you're referring to here. Can you be a bit more specific?

Sure, in https://github.com/mui-org/material-ui-pickers/pull/1730 @dmtrKovalenko is working or removing the built-in en-US error messages as well as the built-in handling of the display of the error in the TextField. We are going into this direction to reduce the scope of the component, do less, do it better, leave developers more flexibility. So my initial request would be:

Would the following API be enough for your mui-rff?

onError?: (reason: 'invalidDate' | 'maxDate' | …, value: any) => void;

people have different opinions on when to apply validation (open #186 and #191)

Yeah, it's a great topic. We have been discussing it internally last week. A summary of the "facts" that came out:

React hook form, Formik, and Final Form wait for the touched state before displaying the error in their validation demos.
Angular describes the why for 'touched' in https://angular.io/guide/form-validation#why-check-dirty-and-touched.

You may not want your application to display errors before the user has a chance to edit the form. The checks for dirty and touched prevent errors from showing until the user does one of two things: changes the value, turning the control dirty; or blurs the form control element, setting the control to touched.

There is an article that goes into more detail on when we would want to wait for the touched state to display the error and when not: https://medium.com/@andrew.burton/form-validation-best-practices-8e3bec7d0549.

Some validations should be displayed as soon as possible, for instance, picking a date in the future.
I believe the best case for the date picker is to display 'invalidDate' only if touched is true (to not interrupt the user while he's filling in the textbox), and to display all the other errors instantly (to reduce context switching for the user).

provide my idea of a sane default and then externalize in the API

Agree, the topic is too sensitive and important to force a behavior. Developers should be able to customize it. You can head to how Angular and the Google products' solve this problem to get this sensitive default. I have personally been using this logic, exclusively:

function RFTextField(props) {
  const {
    autoComplete,
    helperText,
    input: { name, ...input },
    InputProps,
    meta: { dirty, error, submitError, submitFailed },
    ...other
  } = props

  return (
    <TextField
      error={Boolean((dirty || submitFailed) && (error || submitError))}
      {...input}
      {...other}
      id={name}
      name={name}
      InputProps={{
        inputProps: {
          autoComplete,
        },
        ...InputProps,
      }}
      helperText={dirty || submitFailed ? error || submitError : helperText}
    />
  )
}

Finally... I'd love to switch to the new date pickers once things stabilize a bit. I don't have the time/energy to maintain two versions of this project just for date pickers, so that is why I'm still not there yet. Once I cut the next major release to support that, I'd like to put the prior (current) one on (low) maintenance mode.

I think that once the date picker will be inside https://github.com/mui-org/material-ui released under v5 stable, it will be pretty stable :). It will take us some time to get there.

lookfirst commented 4 years ago

Excellent response.

Yes, that API would likely work, but I'd need to really implement something to test it.

I don't have anything to add other than you show one component (TextField)... but each form component has its own validation logic for displaying errors. It is not a one size fits all.

Also don't get me started on more complex form validation... that's how you end up with a whole DSL for validation which just complicates things endlessly. I often wonder if I should just kill this project and point people at DDF, but then I remember that, I and many others, like simple solutions.

CarsonF commented 4 years ago

@oliviertassinari I'd like to speak up for final form. I don't use this library but this this seemed like an appropriate place to put it. Happy to move and start a new thread if desired.

final form handles error states and determines this by validator functions (which are basically (currentValue: any) => string | undefined). The library really pushes this declarative API and while imperative changes can be done, they are IMO workarounds.

Because of this, the new onError property is really hard to integrate with. Here's what happens:

onChange triggered (piped to FF)
FF calls validators and determines new state
onError triggered
now we know there's an error but the validation has already ran for the change.

What would be more helpful is to have an independent function for determining the error reason based on the value and the other constraints. This could be easily decorated into a validator for the field.

lookfirst commented 4 years ago

@CarsonF It is fine here. =) Keeps me in the loop. Great feedback.

oliviertassinari commented 4 years ago

@CarsonF We have observed this exact potential struggle when working on the onError API. Let's say we have prioritized the integration with react-hook-form and formik than react-final-form. Regarding the solution, I think that it could be solved by making the value of the date input richer. Meaning, instead of a pain date type, it could be a { validity: x, value: y } object. The onError callback would trigger the onChange callback with the new validity state. cc @dmtrKovalenko.

CarsonF commented 4 years ago

I think that's a step in the right direction. That would make the validity available for the validators when they need it.

I think as far as FF is concerned the value should still be the date object though, which is what is passed in submit object, used for dirty checks, etc. This means we would need to store that validity separately from FF. While not the simplest, I think that could work.

The validity will need to be stored in a Ref or with something like useGetSet. This is because FF doesn't re-render the Field when the validators change (source) for performance.

oliviertassinari commented 4 years ago

@CarsonF Can allValues be used to store the validity? Do you know any other prior-art on this problem?

CarsonF commented 4 years ago

allValues is just the values of the other fields. It's useful for validating dependencies and business rules. Probably not going to be helpful for this.

I haven't really found any prior-art for this. As stated in this thread, integration with these complex fields comes with a lot of opinions. That's usually why I write them myself. So obviously we want to try and get it most of the way there for them and still allow that customization.

The tricky thing here is we want it to be completely independent of the form and its configuration. So most of the ways FF suggests to customize is out the door. For example, lots can be done with their mutators and decorators but those have to be registered at the Form level. Here's a mutator example: final-form-set-field-data

I think we pretty much have to follow the rules of FF here: Field validity/errors are communicated through validators. However, validators can be called whenever the form deems fit. If the field value is programmatically changed, then FF calls the validator to determine if the new value is valid. But that wouldn't trigger a onChange call because it wasn't a user change. So if a validator wasn't pure and relied on some other state it could become stale and give an incorrect result.

Given all this, I think giving validity onChange won't work.

I think our only choice is to able to call a function to determine that ourselves. That doesn't sound that bad though. Consider this:

const validator = useValidation(); // could use useUtils from context. etc.

const myFinalFormValidator = (value: T) => {
  const reason = validator(value);
  if (!reason) {
    return undefined;
  }
  if (reason === 'minDate') {
    return { reason, message: 'Date is too far in the past' };
  }
  ...
};

return (
  <Field validate={myFinalFormValidator} {...} />
);

I omitted a bunch of connecting logic there with Field and MUI TextField setup. TextField.error can be determined based on field.error.reason allowing for immediate and delayed UI based on type of error. And field.error.message can easily be dropped into to helperText.

That seems pretty easy for the pickers library to provide, and it seems pretty easy for libraries like this to use and further abstract.

<DateField
  errorMessage={{
    minDate: 'Date is too far in the past',
  }}
  showError={{
    invalid: 'AFTER_TOUCHED',
    minDate: 'IMMEDIATELY',
  }}
/>

@oliviertassinari @lookfirst Thoughts?

lookfirst commented 4 years ago

I built this covid survey tool recently using MUI (with a creative use of the Steps component) and MUI-RFF...

https://survey.covid.md/

Go through it and come back.

The cool thing about it is that it builds everything in the UI from a json file...

https://survey.covid.md/model.json

One thing about it is that some of the steps might have a multiple choice set of checkboxes where the last one is 'None of above'. If someone checks that, I'd want all the other checkboxes to be cleared out.

For whatever reason, I ended up not finishing the project, but I got about 80% of the way towards adding that functionality in a generic way. It wasn't easy at all and I still wasn't happy with the implementation.

I guess what I'm trying to say is that the way I work is to implement something real world and not be afraid to rewrite it 50 times until I get it right. That's how I do API design and figure stuff like this discussion out. It isn't ideal to the way guys are discussing things, so I apologize.

I'm happy to just sit back and listen here and see what you guys come up with. Then I'll try it and give feedback. =)

CarsonF commented 4 years ago

@oliviertassinari @dmtrKovalenko bump for my proposal 😄

oliviertassinari commented 4 years ago

@CarsonF The proposal has some potential. I assume that useValidation() would need to receive the exact same arguments as the DatePicker's props. I also assume that useValidation() would need to be used internally by the DatePicker to reduce bundle size and more importantly reduce the bug surface area. Guess what, we have a makeValidationHook method internally that could fit as a solution.

import DatePicker, { useDateValidation } from '@material-ui/pickers/DatePicker';
import DateRagePicker, { useDateRangeValidation } from '@material-ui/pickers/DateRagePicker';
// etc

CarsonF commented 4 years ago

I'm working through integration right now. I have it working with by calling validateDate myself. Except it's not exported so I had to copy the source into my codebase. Even if that was exported it would make it easier. I started looking at ways I could adapt makeValidationHook to work for me. But I abandoned that thought, since it wasn't exported and mostly just doing the onError logic, which I'm ignoring. The lower-level validateDate works great. I could link to src, if it would help, once I push.

lookfirst commented 2 years ago

I'm going to close this as this likely falls under MUI 5.

https://github.com/lookfirst/mui-rff/issues/401

oliviertassinari commented 2 years ago

Yes, sounds great

lookfirst / mui-rff

Material-ui-pickers v4 #106