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

DatePicker not respecting format strings #151

Open
karlshea opened this issue Jul 7, 2020 · 5 comments
Open

DatePicker not respecting format strings #151

karlshea opened this issue Jul 7, 2020 · 5 comments

Comments

@karlshea
Copy link

karlshea commented Jul 7, 2020

It looks like the format string in DatePicker isn't respected when setting form values. I have a picker for a date-only field (passing YYYY-MM-DD into initial form values, and expecting one back), but the value in the submit handler is a full ISO string. This is leading to timezone issues where there shouldn't be any tz information at all.

It also can't be solved with an onChange for the field, because both the moment object and the date string are still correct when that's run. I traced it back to this package's field, which sets the form's field value by calling toISOString() on the moment date object.

Before I make a PR, I'd like some feedback on my solution (or if that's breaking this can be an example for anyone else that's running into the same issue):

const DatePicker = ({
  name,
  validate,
  onChange,
  fast,
  ...restProps
}: DatePickerProps) => (
  <Field name={name} validate={validate} fast={fast}>
    {({
      field: { value },
      form: { setFieldValue, setFieldTouched },
    }: FieldProps) => (
      <$DatePicker
        value={value ? moment(value) : undefined}
        onChange={(date, dateString) => {
// CHANGE START
          const format = date ? date.creationData().format : null;
          setFieldValue(name, date ? (format ? date.format(format.toString()) : date.toISOString()) : null);
// CHANGE END
          setFieldTouched(name, true, false);
          onChange && onChange(date, dateString);
        }}
        {...restProps}
      />
    )}
  </Field>
);

The date moment object contains the format string that antd is using in the picker to format the date, so if it's there I can format the date using the same string on the way out, and it falls back to toISOString() if it's not found.

@jannikbuschke
Copy link
Owner

jannikbuschke commented Jul 8, 2020

Good old time issues :)

Mh, I'm not so sure about your solution. I probably need some time to think about the problem and the solution. Also ant d now supports other datetime libraries (like dayjs), something that maybe should also be considered.

Wouldnt it make sense for you to adjust your datestring before giving it to formik? So internally the component handles a "iso" date, maybe as a UTC+0 date. when you submit you just remove this information again.

// I know, this is not ideal, but I also know that dealing with time stuff is inherently tricky. Your solution might make your use case a bit easier, but the implementation now is trickier to understand and to reason about.

@karlshea
Copy link
Author

karlshea commented Jul 8, 2020

Wouldnt it make sense for you to adjust your datestring before giving it to formik?

Isn't that what this is doing? The onChange handler here is the only place to access the antd component's moment instance before it gets passed to formik.

It would definitely be better if internally the component used a UTC date, because that would make it easier to do tz adjustments afterwards (maybe?), but it seems like either way there should be a better way to manipulate the date before it hits formik state. Because right now a date can't make a round-trip without being changed if your browser isn't at UTC+0.

Maybe there should be another callback between onChange and setFieldValue?

You're right, dealing with date/time is always a pain! Thanks for taking a look at this.

@damonsmith
Copy link

To me it looks like the antd datepicker returns both the date object and the formatted string, but formik-antd takes only the date object, does an toISOString() on it and then always returns that as the value, but that introduces a whole bunch of time and timezone related issues that I would have to solve by parsing and re-formatting the date object.

I'm thinking it would be an option to add a property to the formik-antd to choose whether the form field is the date object or the formatted string. Shall I do a PR for that?

@karlshea
Copy link
Author

In the meantime I've moved away from Antd, so I'm not invested in the outcome of this issue. It can be closed if no one else is running into this problem.

@damonsmith
Copy link

I've raised a PR for this: #175

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants