iTwin / iTwinUI

A design system for building beautiful and well-working web interfaces.
https://itwin.github.io/iTwinUI/
MIT License
101 stars 37 forks source link

Date picker: Time information on date ranges can be lost #1297

Open SpencerWBarnes opened 1 year ago

SpencerWBarnes commented 1 year ago

Describe the bug (actual behavior)

The time component of the start date can be replaced by the end date's time component and vice versa. Once this occurs that information is lost and all future selections will keep the changed time.

Expected behavior

The start date maintains its time component unless specifically changed, and the end date maintains its time component unless specifically changed.

Reproduction

Link to a minimal repro: https://codesandbox.io/s/blissful-euclid-eoyvqq?file=/src/App.tsx

Steps to reproduce end date time loss
  1. With the date picker in "start date selection" mode, click on a date on or after the selected end date
  2. Observe that the selected end date's time component has been replaced with the start date's time component
Steps to reproduce start date time loss
  1. With the date picker in "end date selection" mode, click on a date before the selected start date
  2. Observe that the selected start date's time component has been replaced with the end date's time component

Additional information

The end date time information is lost on line 403 with setSelectedEndDay(newStartDate) and the start date time information is lost on line 431 with setSelectedStartDay(newEndDate). The component also does not handle cases in which the end date's time is before the start date's time (ex: https://codesandbox.io/s/nifty-field-ofs3tz?file=/src/App.tsx)

mayank99 commented 1 year ago

What is the use case for preserving time? It is a date picker, so I expect all times to be irrelevant and always be reset to midnight.


The component also does not handle cases in which the end date's time is before the start date's time

We support disabling certain dates using the isDateDisabled prop. Maybe you can disable all dates before the start-date.


With that said, I would actually recommend using two separate date pickers for the "Start" and "End" dates. We have had multiple reports that the date range picker feels confusing to use, so we might remove it in the future.

You can find an example of separate date pickers in our Table filters. https://itwin.github.io/iTwinUI/react/?path=/story/core-table--filters

SpencerWBarnes commented 1 year ago

The use case is our date picker selects the whole range of time, so the start date is at midnight and the end date is 23:59. Since it is a date picker that takes Date objects as parameters, I expected it to honor the other properties and only change the date aspect of the objects. It would be nice if it could correctly only change the date aspect, but if not then having the documentation mention that the time aspect is not reliable would be enough.


That's not a use case we have right now. It was just one that I observed could occur from the way the source code is written.


Thanks for pointing this out, we will have to revisit which approach we want to use and see what our users think.

mayank99 commented 1 year ago

Could you elaborate a little more on the use case, perhaps with an example? I'd expect the application code to only use the date portion of the dates returned by DatePicker. Does it require a lot of extra code for you to discard the time information?

Lets also look at it from the perspective of the end-user. How would they, when operating the date-picker, know what the time for the "End" date is? All they see is a date, so it makes sense that the time portion is not relevant.

We can improve the documentation for sure, as none of this is clear right now.

SpencerWBarnes commented 1 year ago

In my case, I can make it reliable by setting the start date's time to midnight and the end date's time to end-of-day before returning the values. The end user selects the date range of records they wish to see, so they expect it to be selecting everything from the beginning of the start day to the end of the end day. This only stood out as a bug since it was undocumented and only occurs under certain situations, leading to an unexpected behavior that seemed intermittent.