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

docs: date picker spec #388

Merged
merged 6 commits into from
Apr 4, 2024

Conversation

jnumainville
Copy link
Collaborator

@jnumainville jnumainville commented Mar 25, 2024

Fixes #349

Adds the spec for date picker.
I attempted to emulate React Spectrum's date type specific rendering but with deephaven date types.

@jnumainville jnumainville self-assigned this Mar 25, 2024
@dsmmcken dsmmcken requested a review from alexpeters1208 March 25, 2024 18:07
@dsmmcken
Copy link
Contributor

I think this looks right, but I am not super familiar with our DateTime methods. Adding @alexpeters1208 in case he has any commentary.

Copy link
Contributor

@dsmmcken dsmmcken left a comment

Choose a reason for hiding this comment

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

What is the return type of on_change and how is that controlled?


```py
import deephaven.ui as ui
ui.date_picker(
Copy link
Contributor

Choose a reason for hiding this comment

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

I am surprised that this doesn't support a parameter combination like ui.date_picker(table: Table, date_col: str). I would expect to be able to pass a table in along with a column name for a column containing one of the valid types, and have the picker automatically provide the allowable date ranges accordingly. Not sure what the right default would be, maybe the entire date range?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mofojed @dsmmcken thoughts?
I was thinking about a table source but thought it didn't make much sense due to differing resolutions. I suppose making the column an alternative to min_value or max_value is possible, but seems to be a lot of work for something that could be done through a combination of table operations and use_cell_data hooks.

Copy link
Member

Choose a reason for hiding this comment

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

Yea @alexpeters1208 for that case, wouldn't a ui.picker make more sense instead when selecting a date from a table? As for a range, yea I think use_cell_data would work fine for that case... we should mock out an example though.

Copy link
Contributor

Choose a reason for hiding this comment

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

https://react-spectrum.adobe.com/react-spectrum/DateRangePicker.html is a different thing, but also maybe relevant to the discussion.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mofojed My understanding is that date_picker gives you a kind of calendar pop-up, while picker is a drop-down list. I don't think I'd want to use a drop down list for dates, seems like that list gets really long really quick. Or maybe I'm not understanding?

Copy link
Collaborator Author

@jnumainville jnumainville Mar 28, 2024

Choose a reason for hiding this comment

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

This is a fairly contrived and simplified example but represents how you could derive a value from a table for a range already. It's not difficult to do the end range too but I wanted to show that there is lots of flexibility that might be desired that I think is best achieved outside of the date_picker itself.

table = time_table(period="PT2S")
start = table.head(1)

min_value = ui.use_cell_data(start)
max_value = "2024-29-03"

date_picker = ui.date_picker(min_value=min_value, max_value=max_value)

Copy link
Contributor

Choose a reason for hiding this comment

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

I know you've excluded it, but the isDateUnavailable prop feels kinda like something that could be a column.

Copy link
Collaborator Author

@jnumainville jnumainville Mar 28, 2024

Choose a reason for hiding this comment

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

I had the same thought but quickly ran into trouble when considering how exactly that would be implemented. It doesn't make much sense as a separate boolean column because you would have to check the boolean + date column.
This brings me to the possibility of having a table for unavailable_values with a date column (which I basically emulate with a list of dates at the moment) but is it worth implementing table backing for that?

plugins/ui/DESIGN.md Show resolved Hide resolved
plugins/ui/DESIGN.md Show resolved Hide resolved
@jnumainville
Copy link
Collaborator Author

jnumainville commented Mar 25, 2024

What is the return type of on_change and how is that controlled?

It is the same type that is passed to value, default_value, placeholder_value, or Instant if none of those are specified. My thought was the type would be checked on the python side, serialized, then the callback would be wrapped with a type conversion so that the value passed to the user-specified callback is the same as the input type passed in.

@dsmmcken
Copy link
Contributor

the same type that is passed in

makes sense, and what I expected.

Instant if none of those are specified

Record that decision in your spec.

Copy link
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

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

Crap, I never posted my pending review. Posting now.

plugins/ui/DESIGN.md Outdated Show resolved Hide resolved
plugins/ui/DESIGN.md Outdated Show resolved Hide resolved
plugins/ui/DESIGN.md Outdated Show resolved Hide resolved
plugins/ui/DESIGN.md Outdated Show resolved Hide resolved
Copy link
Contributor

@bmingles bmingles left a comment

Choose a reason for hiding this comment

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

LGTM

@jnumainville
Copy link
Collaborator Author

Going to go ahead and merge with the note that BusinessCalendar could prove useful in the future but will add to spec if needed.

@jnumainville jnumainville merged commit e1a135d into deephaven:main Apr 4, 2024
14 checks passed
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

Successfully merging this pull request may close these issues.

ui.date_picker spec
5 participants