feat: Javascript DatePicker Implementation #667

dgodinez-dh · 2024-07-25T15:33:08Z

Closes #369
Implements JS side of DatePicker.

plugins/ui/src/js/src/elements/hooks/useDatepickerProps.ts

bmingles

Code generally looks good to me. Left a few comments / questions.

bmingles

I would still suggest using the re-exported MappedDateValue, but I don't feel strongly enough to block the PR for it. Everything looks good to me.

AkshatJawne · 2024-07-26T14:20:53Z

plugins/ui/src/js/src/elements/DatePicker.tsx

This code looks good, although after my ui.text_area PR merges, we should probably create a follow-up ticket to leverage the useDebouncedOnChange hook I created, since this logic here to handle input looks similar: https://github.com/deephaven/deephaven-plugins/blob/0f0f3ca7d8e3016182f89127ceca677d09fcb27e/plugins/ui/src/js/src/elements/hooks/useDebouncedOnChange.ts

Doesn't impact this PR merging though 👍🏼

@AkshatJawne that hook should probably live in DHC instead of plugins repo.

Co-authored-by: Mike Bender <[email protected]>

chipkent · 2024-08-07T16:33:50Z

plugins/ui/docs/components/date_picker.md

+zoned_date_time = to_j_zdt("1995-03-22T11:11:11.23142 UTC")
+instant = to_j_instant("2022-01-01T00:00:00 ET")


Not a great example because the ZDT is UTC, which looks like the same behavior as Instant.

Fixed in an update.

chipkent · 2024-08-07T17:24:41Z

plugins/ui/docs/components/date_picker.md

This interface feels like it has an incomplete integration with the existing time libraries. In particular, business calendars are not supported. Business calendars could be used to limit selected dates or times, they can be used to get a appropriate time zone, etc. Business calendars could be added as a future enhancement, but I wanted to note it here.
https://deephaven.io/core/pydoc/code/deephaven.calendar.html
https://github.com/deephaven/deephaven-core/blob/main/engine/time/src/main/java/io/deephaven/time/calendar/BusinessCalendar.java

We're looking at adding the unavailable dates afterwards: #698
Didn't want that to hold up the the rest of the date picker.

chipkent · 2024-08-07T17:31:15Z

plugins/ui/docs/components/date_picker.md

date_picker interprets date-time strings in a way that is inconsistent with all other DH time libraries. For example, it interprets any UTC string only as an Instant. All other time libraries accept the same date-time format for either an Instant or a ZonedDateTime. The inconsistency introduced by date_picker is not good and will be confusing to explain and support. I think the inconsistency can be resolved cleanly by introducing a date_type or string_format parameter that can take values of LocalDate, Instant, and ZonedDateTime. This could then be used to control how the string is parsed.

Note: I have a PR to introduce a new format string that is ZDT specific, but there has been enough arguing that I have not pushed it forward. deephaven/deephaven-core#5069

Per discussion, we are going to default datetime strings to ZDT. Then update the documentation to clarify which value input types will produce which output types.

chipkent · 2024-08-07T17:35:05Z

plugins/ui/docs/components/date_picker.md

I don't think the widget has a way to select a time zone via clicking. I can imagine that this would be useful. The adobe docs don't seem to mention it, so it might not be available.

Yes, the DatePicker will display a timezone but does not allow to change it.
For the current implementation, the DatePicker will display the TZ from the user settings and update if the user changes the TZ in the settings. This is for an Instant. If the user uses a ZDT, then the DatePicker is locked to the TZ provided by the ZDT.

chipkent · 2024-08-07T17:37:47Z

plugins/ui/docs/components/date_picker.md

+1. `value`
+2. `default_value`
+3. `placeholder_value`


Do they get an error if value and default_value are inconsistent types?

I tried this in various permutations.
There is no error. Instead one of the types "wins" in order of value, default_value, placeholder_value
The docs discuss this as on_change being determined in that precedence.

chipkent · 2024-08-07T17:43:46Z

plugins/ui/docs/components/date_picker.md

+Recommendations for creating clear and effective date pickers:
+
+1. Using the Date Type `Instant` will be most compatible with other Deephaven components.


This is a weird recommendation because doing everything in UTC is the least useful presentation for a user.

Removed in an update.

chipkent · 2024-08-07T17:45:13Z

plugins/ui/docs/components/date_picker.md

+        ui.date_picker(description="description"),
+        ui.date_picker(error_message="error", validation_state="valid"),
+        ui.date_picker(error_message="error", validation_state="invalid"),
+        ui.date_picker(min_value="2024-01-01", max_value="2024-01-5"),


2024-01-5 looks like it should be an invalid ISO date.
https://en.wikipedia.org/wiki/ISO_8601#Calendar_dates

Fixed in an update.

chipkent · 2024-08-07T17:52:08Z

plugins/ui/docs/components/date_picker.md

+
+1. `value`
+2. `default_value`
+3. `placeholder_value`


In DESIGN.md, the default is described as Defaults to today at midnight in the user's timezone. I assume that doc should also indicate that it is a ZonedDateTime. There is an enormous amount of ambiguity in what the "user's timezone" is. Possibilities are:

The local machine time zone.

The Java default time zone.

The time zone of the default calendar.

The time zone configured for rendering in the UI.

There needs to be a discussion about which is most appropriate and for what reason. The docs then need to be very clear on the default.

Notes:

https://deephaven.io/core/docs/conceptual/time-in-deephaven/#time-zones-and-aliases

This comes from the Spectrum docs:

A placeholder date that influences the format of the placeholder shown when no value is selected. Defaults to today's date at midnight.

So it must be defaulting to the local machine time zone.

chipkent · 2024-08-07T18:16:48Z

plugins/ui/docs/components/date_picker.md

There is a question about what return values should result from string inputs. There was a very long discussion around this topic when we created the query language time and calendar library. Ultimately, we decided to make everything as java time centric as possible. As a result, almost all return types are Java time types. There are a few cases where local-date-strings can be provided as inputs and they will return local-date-strings as outputs, but I think these were only included to make it easier to work with tables using strings as date columns -- which is very common.

As a result, I think it is least confusing to always return a java time value. If there is a compelling case to do something different, we should discuss that specific case.

Having said that, it doesn't look like we have exposed the string formatting methods (e.g. https://github.com/deephaven/deephaven-core/blob/main/engine/time/src/main/java/io/deephaven/time/DateTimeUtils.java#L4273) in python. (https://deephaven.io/core/pydoc/code/deephaven.time.html). Do we need to do this?

Notes:

https://github.com/deephaven/deephaven-core/blob/main/engine/time/src/main/java/io/deephaven/time/DateTimeUtils.java

https://github.com/deephaven/deephaven-core/blob/main/engine/time/src/main/java/io/deephaven/time/calendar/Calendar.java

https://github.com/deephaven/deephaven-core/blob/main/engine/time/src/main/java/io/deephaven/time/calendar/BusinessCalendar.java

Per discussion, we are going to stick with Java date types. User can convert to python / strings as needed.

chipkent · 2024-08-07T18:20:42Z

plugins/ui/docs/components/date_picker.md

+
+## Date types
+
+There are three types that can be passed in to the props that control the date format:


Only supporting these 3 inputs types is probably excessively restrictive. Our to_j_* methods can handle far more types. For example, to_j_instant can take Instant, int, str, datetime.datetime, numpy.datetime64, and pandas.Timestamp inputs. to_local_date can take about the same inputs, so supporting all of these types would probably need the date_type parameter suggested above.

Per discussion, we do support those other input types. Will need to update docs to clarify inputs and outputs.

mofojed

It's still showing up as GMT-5 for me when I enter America/New_York for my time zone:

Shouldn't that say EST/EDT? It's weird when I switch dates and it switches from GMT-5 to GMT-4...

mofojed · 2024-08-19T14:44:41Z

It's still showing up as GMT-5 for me when I enter America/New_York for my time zone:
Turns out this was because in Chrome I had "English (United Kingdom)" set as my preferred language in settings: It's still showing up as GMT-5 for me when I enter America/New_York for my time zone:
Not sure why that was set to me. Changing to "English (Canada)" shows "EST/EDT" correctly.

dsmmcken · 2024-08-19T18:05:29Z

plugins/ui/docs/components/date_picker.md

+## Variants
+
+Date Pickers can have different variants to indicate their purpose.
+
+```python
+from deephaven import ui
+
+
+@ui.component
+def date_picker_variants():
+    return [
+        ui.date_picker(description="description"),
+        ui.date_picker(error_message="error", validation_state="valid"),
+        ui.date_picker(error_message="error", validation_state="invalid"),
+        ui.date_picker(min_value="2024-01-01", max_value="2024-01-05"),
+        ui.date_picker(value="2024-07-27T16:10:10 America/New_York", hour_cycle=24),
+        ui.date_picker(granularity="YEAR"),
+        ui.date_picker(granularity="MONTH"),
+        ui.date_picker(granularity="DAY"),
+        ui.date_picker(granularity="HOUR"),
+        ui.date_picker(granularity="MINUTE"),
+        ui.date_picker(granularity="SECOND"),
+    ]
+
+
+date_picker_variants_example = date_picker_variants()
+```


This should be split into timezone, validation and granulariy sections.

Docs are expected to be exhaustive and there are several concepts and sections from: https://react-spectrum.adobe.com/react-spectrum/DatePicker.html that are missing

This reverts commit 183b620.

dgodinez-dh added 4 commits July 24, 2024 14:07

ui.date_picker JS side implementation

6d8433e

install internationalized date

72fdecf

deserialize value props

70a3886

isUnavailable callback

f5b48ef

bmingles reviewed Jul 25, 2024

View reviewed changes

plugins/ui/src/js/src/elements/hooks/useDatepickerProps.ts Show resolved Hide resolved

bmingles reviewed Jul 25, 2024

View reviewed changes

plugins/ui/src/js/src/elements/hooks/useDatepickerProps.ts Outdated Show resolved Hide resolved

bmingles reviewed Jul 25, 2024

View reviewed changes

plugins/ui/src/js/src/elements/hooks/useDatepickerProps.ts Outdated Show resolved Hide resolved

bmingles reviewed Jul 25, 2024

View reviewed changes

plugins/ui/src/js/src/elements/hooks/useDatepickerProps.ts Outdated Show resolved Hide resolved

bmingles reviewed Jul 25, 2024

View reviewed changes

plugins/ui/src/js/src/elements/hooks/useDatepickerProps.ts Show resolved Hide resolved

bmingles requested changes Jul 25, 2024

View reviewed changes

review suggestions and unit test

73131cc

bmingles previously approved these changes Jul 25, 2024

View reviewed changes

AkshatJawne previously approved these changes Jul 26, 2024

View reviewed changes

fix nullable types

8f86f73

dgodinez-dh dismissed stale reviews from AkshatJawne and bmingles via 8f86f73 July 26, 2024 14:37

dgodinez-dh closed this Jul 29, 2024

dgodinez-dh added 11 commits July 29, 2024 09:33

make component annotation

1bc8a2b

dates to iso strings

5f7663d

add is_instant_string

fbdf5c8

use time zone setting

924c795

update unit tests

863e20d

comment out unavailable values

cbf58d2

doc updates

79b1ee4

merge latest

438e1ba

fix granularity

c0ca90e

date picker doc

0d6f1a9

remove commented code

e3bf54b

dgodinez-dh reopened this Jul 30, 2024

dgodinez-dh marked this pull request as ready for review July 30, 2024 19:58

dgodinez-dh and others added 3 commits August 6, 2024 09:09

Update plugins/ui/docs/components/date_picker.md

e5d65e1

Co-authored-by: Mike Bender <[email protected]>

Update plugins/ui/docs/components/date_picker.md

b1c1fba

Co-authored-by: Mike Bender <[email protected]>

fix wrapped date callable

4784e12

chipkent reviewed Aug 7, 2024

View reviewed changes

dgodinez-dh marked this pull request as draft August 7, 2024 19:34

dgodinez-dh added 8 commits August 7, 2024 14:07

update docs

afdf9a4

merge latest

d97643e

more doc updates

84ea457

string parsing defaults to zdt

3b1bc5e

fix unit test

a68597d

more doc updates

ea349ce

more examples

dac5a6f

doc example fixes

c2f00c5

dgodinez-dh marked this pull request as ready for review August 8, 2024 12:47

merge latest

4fd4365

mofojed reviewed Aug 19, 2024

View reviewed changes

mofojed previously approved these changes Aug 19, 2024

View reviewed changes

server side placeholder default

910d544

dgodinez-dh dismissed mofojed’s stale review via 910d544 August 19, 2024 16:34

merge latest

5462e82

dsmmcken reviewed Aug 19, 2024

View reviewed changes

dgodinez-dh added 4 commits August 19, 2024 12:20

handle time zone for uncontrolled date pickers

7a86f21

update images

183b620

Revert "update images"

9c6a007

This reverts commit 183b620.

update images

c399f1c

dgodinez-dh requested a review from mofojed August 19, 2024 19:19

mofojed approved these changes Aug 21, 2024

View reviewed changes

dgodinez-dh merged commit ff48512 into deephaven:main Aug 21, 2024
16 checks passed

dgodinez-dh deleted the dag_DatePickerUi branch August 21, 2024 13:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Javascript DatePicker Implementation #667

feat: Javascript DatePicker Implementation #667

dgodinez-dh commented Jul 25, 2024 •

edited

Loading

bmingles left a comment

bmingles left a comment

AkshatJawne Jul 26, 2024

bmingles Jul 26, 2024

chipkent Aug 7, 2024

dgodinez-dh Aug 7, 2024

chipkent Aug 7, 2024

mofojed Aug 7, 2024

chipkent Aug 7, 2024

dgodinez-dh Aug 7, 2024

chipkent Aug 7, 2024

dgodinez-dh Aug 7, 2024

chipkent Aug 7, 2024

dgodinez-dh Aug 7, 2024

chipkent Aug 7, 2024

dgodinez-dh Aug 7, 2024

chipkent Aug 7, 2024

dgodinez-dh Aug 7, 2024

chipkent Aug 7, 2024

dgodinez-dh Aug 7, 2024

chipkent Aug 7, 2024

dgodinez-dh Aug 7, 2024

chipkent Aug 7, 2024

dgodinez-dh Aug 7, 2024

mofojed left a comment

mofojed commented Aug 19, 2024

dsmmcken Aug 19, 2024

		zoned_date_time = to_j_zdt("1995-03-22T11:11:11.23142 UTC")
		instant = to_j_instant("2022-01-01T00:00:00 ET")

		Recommendations for creating clear and effective date pickers:

		1. Using the Date Type `Instant` will be most compatible with other Deephaven components.


		## Date types

		There are three types that can be passed in to the props that control the date format:

feat: Javascript DatePicker Implementation #667

feat: Javascript DatePicker Implementation #667

Conversation

dgodinez-dh commented Jul 25, 2024 • edited Loading

bmingles left a comment

Choose a reason for hiding this comment

bmingles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mofojed left a comment

Choose a reason for hiding this comment

mofojed commented Aug 19, 2024

Choose a reason for hiding this comment

dgodinez-dh commented Jul 25, 2024 •

edited

Loading