duetds / date-picker

Duet Date Picker is an open source version of Duet Design System’s accessible date picker. Try live example at https://duetds.github.io/date-picker/
https://www.duetds.com
MIT License
1.73k stars 68 forks source link

[Bug] Date Picker doesn't update value after an invalid date was typed into the input #47

Open sto3psl opened 4 years ago

sto3psl commented 4 years ago

Describe the bug The value of <duet-date-picker> is not updated when a valid date is changed to an invalid one. This can cause a form to submit with the former valid date instead of erroring or submitting the actually entered invalid date.

To Reproduce Steps to reproduce the behavior:

  1. Go to https://codesandbox.io/s/silly-sunset-cvxkq
  2. Enter a valid date into the date picker input (eg. 2020-11-12)
  3. Submit the form
  4. See that submitted value and actual value are the same.
image
  1. Change date picker input to an invalid date (eg. 2020-11-121244345)
  2. Submit the form
  3. See that submitted value is still the old one and not the one entered.
image

Expected behavior I'm not sure what the expected behavior is. Maybe an empty string or something like INVALID DATE should get submitted. That way my submit handler can validate against bad inputs.

Additional context Version @duetds/date-picker@1.1.0

I think the problem is that the hidden input with name=date is only updated when a correct date was entered. Then changing to an invalid date will not trigger an update which causes the hidden input to keep its former valid date, which gets submitted. As described above, my suggestion is to update the hidden input with some kind of invalid date value.

aarongeiser commented 3 years ago

Also encountering this issue. It would be helpful if the focus and blur events emittied the actual current value in the input. That way - whoever is implementing the component can perform their own validation on the value that is being typed. There are other types of validation that may occur beyond simply checking if the date is a valid format. Limitations to date range for instance - or the date in relation to another form field.

WickyNilliams commented 3 years ago

I'm going to add some support for the validity property, like native inputs. This offers all the information you need about valid format, empty value, exceeding min/max etc. This is always kept up to date and can be inspected at any time, so should suffice for all validation needs

WickyNilliams commented 3 years ago

As in implement this API https://developer.mozilla.org/en-US/docs/Web/API/ValidityState

shinkathe commented 3 years ago

@WickyNilliams thank you for your responses, here's my two dollars as our team of about 20 people just implemented a new website using this component and we ran into the same issues:

The problem with the current design is this: There might be a validation and form library or just a higher order component that the component lives within that is interested in its value. If the validation and value storage lives outside (in a higher order component), then in case the user types an invalid value inside the datepicker component, the higher order system is not notified, because the onChange is not invoked. Furthermore, some libraries use onChange as the only source of signaling for when to perform validation.

The specific problem this causes, is that the value that the external system (Formik or such) has stored for the datepicker might be an old value that was valid at some point and when we change that value to an invalid one, date picker resets its value to "" and does not call duet changed which means, that the HOC world is left to the old value and the datepicker value and HOC value is out of sync. Then when we submit the form, the old value is sent and the datepicker is only internally in a failed state. The user has no idea, what value is actually being saved. Therefore, the user should be able to see the value that is saved. OnChange should also always be updated to reflect the value that the user sees. In case of an invalid value, the empty string should also be sent via duetChanged.

This, however, I understand is a breaking change - therefore I'd probably consider adding a new event for this for now, and maybe consider changing this behaviour in a bigger release. I'd be happy to provide a pullrequest for either change.

I would additionally consider making some of the library internals public so we could replace them by inheritance.

Thanks

alexkrauss commented 1 year ago

We are also hitting this issue and I wonder if anyone has found workarounds.

Alternatively, we consider creating a (non-breaking) PR, but would prefer to get maintainer feedback before diving into this.

klh commented 1 year ago

@kasimir-dka could you take a look at this one?, perhaps upstream the internal version to this open-source one?