patternfly / patternfly-design

Use this repo to file all new feature or design change requests for the PatternFly project
114 stars 104 forks source link

Bug - [DatePicker] - [validator function converts date automatically and exports a new date when users input some special wrong date ] #1269

Open judy20191103 opened 1 year ago

judy20191103 commented 1 year ago

Describe the problem PatternFly DatePicker component has converted 2023-06-31 into 2023-07-01 for us, which triggered our weekend validation. Actually, I want to get the true value 2023-06-31, I know its an invalid date, but it's true, it makes sure our logic depends on the users input value.

How do you reproduce the problem? I show a sample demo like below:

<DatePicker validators={[rangeValidator]} />

function rangeValidator(date: Date) { const dayOfWeek = new Date(date).getDay(); const isWeekend = dayOfWeek === 6 || dayOfWeek === 0 console.log("date==", date);// 2023-07-01, 2022-10-01, 2023-05-01 if (isWeekend) { return "Date falls on weekend."; } return ""; }

I have tried other cases like 2022-09-31 => 2022-10-01, 2023-04-31 => 2023-05-01, the DatePicker component converts these dates automatically and exports the later value, resulting in a validation notice.

Expected behavior I want to get the true value the users input, like 2023-06-31

Is this issue blocking you? Not yet

Screenshots If applicable, add screenshots to help explain the issue.

What is your environment?

What is your product and what release date are you targeting?

Any other information?

nicolethoen commented 1 year ago

Would it be helpful if we passed back the original date provided by the user in addition to the converted valid date as part of the callback function? That wouldn't be a breaking change. It would still display the 'corrected' value in the input.

shubhit7 commented 1 year ago

This is not exactly an issue imho. JS 'Date' object automatically tries to convert the invalid date to the nearest valid date.

To handle the use case defined above, can we use the value which gets passed in onChange(_event, value, date) ? Or we disallow the invalid date in the first place, maybe by letting users select from date picker only rather than entering it manually in the text field ?

nicolethoen commented 1 year ago

hm... it's a interesting request since typing an poorly formatted date does trigger the error state, so the only dates we are talking about are the ones that native JS is converting to the equivalent valid date - and we want to be sure that if there is a properly formatted date in the datepicker input, that it corresponds to a valid date in the datepicker's calendar when its opened. I guess the solution could be that we somehow override JS's native conversion and display the datepicker is invalid with an error message.

shubhit7 commented 1 year ago

@nicolethoen I was thinking of a similar approach to display the error message when invalid date is entered. Can we make use of https://github.com/patternfly/patternfly-react/blob/0a289daefea4b42da69c05eb190f85ce90b75142/packages/react-core/src/components/DatePicker/DatePicker.tsx#L143 So on every change event we check the date for its validness, maybe edit the logic in https://github.com/patternfly/patternfly-react/blob/0a289daefea4b42da69c05eb190f85ce90b75142/packages/react-core/src/helpers/datetimeUtils.ts#L4

shubhit7 commented 1 year ago

@nicolethoen I was thinking of a similar approach to display the error message when invalid date is entered. Can we make use of

https://github.com/patternfly/patternfly-react/blob/0a289daefea4b42da69c05eb190f85ce90b75142/packages/react-core/src/components/DatePicker/DatePicker.tsx#L143

So on every change event we check the date for its validness, maybe edit the logic in https://github.com/patternfly/patternfly-react/blob/0a289daefea4b42da69c05eb190f85ce90b75142/packages/react-core/src/helpers/datetimeUtils.ts#L4

I think the datetimeUtils.ts is fine, it receives the date object after conversion. So we may have to edit https://github.com/patternfly/patternfly-react/blob/0a289daefea4b42da69c05eb190f85ce90b75142/packages/react-core/src/components/DatePicker/DatePicker.tsx#L89 This new Date() is actually doing the conversion to the valid date.

nicolethoen commented 1 year ago

I'm going to send this issue over to design for them to consider the ideal interaction for when someone tries to input Sept 31 into the date picker.

shubhit7 commented 1 year ago

@nicolethoen Can we keep the interaction as it is now.

Basically, the dateParse returns false in case of any invalid date format, we can introduce one more check here to also check for the actual invalid date, and if so, the dateParse will return false as it does now.

Would like me to send a PR for review on patternfly-react ?

nicolethoen commented 1 year ago

You are welcome to open a PR! That will give us an example to interact with as we evaluate the interaction/experience.

Ginxo commented 4 weeks ago

I hope it helps, I found the same problem on my side, let me post (inline) the example I have

      <DatePicker
        onChange={(_, dateStr) => {
          setValue(dateStr);
        }}
        onBlur={(_, dateStr) => {
          setValue(dateStr);
        }}
        value={value}
        dateParse={(dateStr: string) => dayjs(dateStr, 'YYYY-MM-DD', true).toDate();}
        validators={(date: Date) => [(date: Date) => (!isValidDate(date) ? 'Invalid date' : '')]} // isValidDate from @patternfly/react-core
        key={key}
      />

the date string value arrives already parsed for a 'YYYY-MM-D' case since the DatePicker seems to execute its own dateParse function (val: string) => (val.split('-').length === 3 ? new Date(${val}T00:00:00) : new Date(undefined))

After a bit of investigation it seems like dateParse function is not passed as a property to the CalendarMonth component and whenever isValidated function is called, new Date is generated without ad-hoc parsing mechanism, see https://github.com/patternfly/patternfly-react/blob/0aff022aa701fb8974fc1887301bfd4e8bf8e732/packages/react-core/src/components/CalendarMonth/CalendarMonth.tsx#L176

I could be wrong, just in case it helps.