Skip to content
This repository has been archived by the owner on May 24, 2024. It is now read-only.

[terra-date-time-picker] Does not honor time in maxDateTime (assume minDateTime as well) #524

Closed
prmeznar opened this issue Feb 13, 2019 · 4 comments · Fixed by #578
Closed

Comments

@prmeznar
Copy link

Bug Report

Description

The terra-date-time-picker calls onChange as long as the date is within the minDateTime/maxDateTime range, even if the time makes those values exceed the minDateTime/maxDatetime.

Steps to Reproduce

With onChange set to print some text to the console

  • Set an max date time halfway through some date (D;T) for a maxValue
  • Pick a valid date time (D-1;T+1), see onChange fire
  • Change the date to an invalid date (D+1,T+1), onChange does not fire
  • Change the date to D (D; T+1), so that the time is invalid, onChange fires, we expect it not to

Expected Behavior

  • onChange is not called when a dateTime outside of min max bounds is entered
  • onChangeRaw is called when a dateTime outside of min max bounds is entered

@ Mentions

@benbcai

@bjankord
Copy link
Contributor

Talked about adding a prop to provide access to the invalid date change, potentially onInvalid or onError.

@JakeLaCombe JakeLaCombe transferred this issue from cerner/terra-core Feb 20, 2019
@bjankord bjankord added this to the Backlog milestone Feb 21, 2019
@prmeznar
Copy link
Author

I think there's a related issue with DateTimePicker and initial values.
We have a common workflow where the initial date and the minimum date are both set to now (D;T).
Whenever the component gains focus, the value is immediately cleared.

This seems to be because underneath the DateTimePicker uses the DatePicker, and the DatePicker strips the time off of the selected date before performing it's comparisons to determine if the selected date is valid, but doesn't strip the time off of the minimum and maximum.

So, you end up comparing D;T to D (the minimum), which fails, so it considers the value as invalid and clears it.

image

I would expect the date picker to either strip time off of both selected date and min/max date time, or neither.

@benbcai benbcai self-assigned this Mar 22, 2019
@benbcai
Copy link
Contributor

benbcai commented Mar 28, 2019

@prmeznar After discussing with our team on how to handle the time aspect in terms of the min/max date/time, it turns out that there are some (edge) cases that are difficult for terra-date-time-picker to manage and would lead to an undesired user experience. For that reason, we plan to remove the minDateTime and maxDateTime props in the next major version. What we recommend consumers to do now and going forward is to use form validation to perform validation on the date/time value and show an error or alert when appropriate.

We still plan to keep the minDate and maxDate props in terra-date-picker. There is still a small change that we will make in the isDateOutOfRange function shown in your above screenshot to make it work as expected when only either the minDate or maxDate (but not both) is passed in.

@benbcai
Copy link
Contributor

benbcai commented Apr 18, 2019

After further discussions with the consumers and terra team, it was decided to not completely remove the minDateTime and maxDateTime props but instead rename the minDateTime and maxDateTime props to minDate and maxDate respectively. If the time portion is included in these props, the time portion would not be taken into consideration when determining the minimum and maximum time. The minDate and maxDate props would straight up be passed down from the date-time-picker to the date-picker and would let the date-picker handle these props. This decision would keep the consistency of having these two props in both the date-picker and date-time-picker and also stays consistent with the includeDates and excludeDates where the time portion is not taken into consideration and would just be passed down directly to the date-picker to reflect the disabled/enabled dates in the calendar picker.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.