Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add date input validators #5024

Closed
wants to merge 17 commits into from

Conversation

roseline124
Copy link

@roseline124 roseline124 commented Jul 8, 2020

Feature Background

We can use custom DateInput with material-ui's DatePicker supporting this feature.
But, it causes additional codes and package and there might be people (like me) who prefers DateInput to DatePicker because of some reasons(UI and format bug, etc). so I add this date validators.

Changes

  • Add date, datetime validators minDate, maxDate
  • Add minDate, maxDate to validation.spec.ts
  • Add translated message (en, fr)
  • Add example in CreateEdit.md document

Check Lists

  • pass unit test
  • pass lint

@roseline124 roseline124 changed the title Add date input validation Add date input validators Jul 8, 2020
Copy link
Member

@fzaninotto fzaninotto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand the difference with minValue and maxValue. Is it only the formatting of the min and max values in the error message?

packages/ra-language-french/src/index.ts Outdated Show resolved Hide resolved
packages/ra-language-french/src/index.ts Outdated Show resolved Hide resolved
@@ -1,4 +1,5 @@
import lodashMemoize from 'lodash/memoize';
import format from 'date-fns/format';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Browsers have an API for formatting dates (Intl), no need for an external library

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, these validators look great 👍

Thanks for contributing 😊.

I've one remark: Can we can consider using another name for these validators?

  • Minimal date means the input should contain a date after the specified one
  • Maximal date means the input should contain a date before the specified one

So maybe you can name these methods afterDate and beforeDate. What do you think?

Hi, these validators look great 👍

Thanks for contributing 😊.

I've one remark: Can we can consider using another name for these validators?

  • Minimal date means the input should contain a date after the specified one
  • Maximal date means the input should contain a date before the specified one

So maybe you can name these methods afterDate and beforeDate. What do you think?

I read your comment. Thank you for reviewing my code!
I implemented code to allow the base date to be included, so validators are named 'min' and 'max'. You means that it'll be better if the code excludes the base date? or just changes the validator name?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Browsers have an API for formatting dates (Intl), no need for an external library

Thank you!
I used new Intl.DateTimeFormat().format(date) also changed format options (DateTimeFormatOptions).
The option can be a little long like this. I don't know this is good.

const options = {
    year: 'numeric',
    month: 'short',
    day: 'numeric',
    hour: 'numeric',
    minute: 'numeric',
};

It needs to know DateTimeFormatOptions and readability doesn't look good.
What do you think?

Copy link
Contributor

@Luwangel Luwangel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, these validators look great 👍

Thanks for contributing 😊.

I've one remark: Can we can consider using another name for these validators?

  • Minimal date means the input should contain a date after the specified one
  • Maximal date means the input should contain a date before the specified one

So maybe you can name these methods afterDate and beforeDate. What do you think?

roseline124 and others added 3 commits July 10, 2020 17:36
Co-authored-by: Francois Zaninotto <fzaninotto@gmail.com>
Co-authored-by: Francois Zaninotto <fzaninotto@gmail.com>
!isEmpty(value) && new Date(value) < new Date(min)
) => {
const inputValue = new Date(value);
inputValue.setSeconds(0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the rationale of this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's written below!

@roseline124
Copy link
Author

roseline124 commented Jul 10, 2020

I'm not sure I understand the difference with minValue and maxValue. Is it only the formatting of the min and max values in the error message?

I admit minValue and maxValue can cover this feature. But, I think minDate, maxDate validators are more readable and the error message is also important, too.

  1. comparing code using minDate to one not using

I added nowDate.setSeconds(0) to my feature.
Because DateTimeInput can't choose seconds, the seconds is set as 0 when selecting date in DateTimeInput.
So If I had set the baseDate as Fri Jul 10 2020 17:47:57 and selected Fri Jul 10 2020 17:47, the validator will throw error even If these are the same date.

<DateTimeInput
  source="advertisementEndAt"
  validate={[minDate(new Date())]}
/>
<DateTimeInput
  source="advertisementEndAt"
  validate={[
    (value, values) => {
      const nowDate = +new Date()
      nowDate.setSeconds(0)
      return minValue(nowDate)(Number(value), values)
    },
  ]}
/>

2. Error message
- minDate: Must be after 2020-07-10
- minValue: Must be at least 1594371535226
-> ah, it could be also formatted like minValue(..., ``Must be after ${formattedValue}``)

I'm okay if you close this PR.
Thank you for reviewing my code

@Luwangel Luwangel added RFR Ready For Review enhancement labels Aug 3, 2020
@@ -946,6 +949,7 @@ const validateEmail = email();
const validateAge = [number(), minValue(18)];
const validateZipCode = regex(/^\d{5}$/, 'Must be a valid Zip Code');
const validateSex = choices(['m', 'f'], 'Must be Male or Female');
const validateDate = [minDate(new Date())];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should use maxDate here to make this example more consistant with a Birthdate and avoid confusion IMO

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should also add formatting options to illustrate usage

@@ -919,6 +919,8 @@ Alternatively, you can specify a `validate` prop directly in `<Input>` component
* `required(message)` if the field is mandatory,
* `minValue(min, message)` to specify a minimum value for integers,
* `maxValue(max, message)` to specify a maximum value for integers,
* `minDate(min, message)` to specify a minimum value for date,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doc is now incomplete - you must add the formatting options parameter

@@ -946,6 +949,7 @@ const validateEmail = email();
const validateAge = [number(), minValue(18)];
const validateZipCode = regex(/^\d{5}$/, 'Must be a valid Zip Code');
const validateSex = choices(['m', 'f'], 'Must be Male or Female');
const validateDate = [minDate(new Date())];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should also add formatting options to illustrate usage

const input = new Date(value);
input.setSeconds(0);

return !isEmpty(value) && input < new Date(min)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

min should be a Date, why do you create a new Date object

* <DateInput name="foo" validate={fooValidators} />
*/
export const minDate = memoize(
(min, message = 'ra.validation.minDate', formatOptions = {}) => (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please type the args using TypeScript

) => {
const input = new Date(value);
input.setSeconds(0);
return !isEmpty(value) && input > new Date(max)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same, max should already be a Date

@fzaninotto fzaninotto closed this Aug 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement RFR Ready For Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants