fix(genui): fix DateTimeInput core catalog item
#622
Conversation
andrewkolos
changed the title
DateTimeInput core catalog item
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves the "DateTimeInput" component by addressing issues like placeholder text, atomic updates, and output formats, and enhances stability with comprehensive tests. However, a medium-severity vulnerability was identified: the new code does not properly validate the length of the "value" before parsing it as a date, which could lead to a client-side Denial of Service due to uncontrolled resource consumption. Additionally, a few minor suggestions have been made to further enhance code readability and maintainability.
packages/genui/lib/src/catalog/core_widgets/date_time_input.dart
Outdated
Show resolved
Hide resolved
| firstDate: DateTime(2000), | ||
| lastDate: DateTime(2100), |
There was a problem hiding this comment.
The firstDate and lastDate for the date picker are hardcoded. To make this component more reusable and flexible, consider allowing these to be configured as properties on the DateTimeInput component. You would need to update the schema and _DateTimeInputData extension type as well, using the current values as defaults.
There was a problem hiding this comment.
Hmmm shall we just make these super broad, e.g make the min year 0 or something? I assume it still opens on the current month etc?
There was a problem hiding this comment.
I changed the range to be [DateTime(-9999), DateTime(9999)], effectively removing the limits. The UI seems to handle this gracefully (year picking widget lazily renders years and will incrementally render more years as you scroll near a boundary).
packages/genui/lib/src/catalog/core_widgets/date_time_input.dart
Outdated
Show resolved
Hide resolved
jacobsimionato
left a comment
jacobsimionato
left a comment
There was a problem hiding this comment.
Hey this looks great! Sorry if it wasn't ready for review yet, but was just driving by :-D.
| firstDate: DateTime(2000), | ||
| lastDate: DateTime(2100), |
There was a problem hiding this comment.
Hmmm shall we just make these super broad, e.g make the min year 0 or something? I assume it still opens on the current month etc?
| "path": "/myTime" | ||
| }, | ||
| "enableDate": false, | ||
| "outputFormat": "time_only" |
There was a problem hiding this comment.
Hmmmm I'm not sure what the intention of this parameter was actually. Perhaps a format string e.g. YYYY-MM-DD ? I'm going to propose removing it from the spec...
For now, maybe we should ignore it?
There was a problem hiding this comment.
Yeah, I imagine it being a format string like you said. The problem that there is no universally accepted datetime formatting string. That could make supporting custom date formatting a very tricky problem for some (many?) renderers.
There was a problem hiding this comment.
Output format was already being ignored here. I went ahead and removed the parameter entirely from the code.
andrewkolos
force-pushed
the
fix-date-time-picker
branch
from
8fe6c88 to
3a9c890
Compare
|
Hey btw I proposed to remove the output format param entirely in google/A2UI#353 . I'll probably quietly merge that if no-one complains! |
2 participants
Description
Fixes #543.
This change fixes quite a few issues with the DateTimeInput core catalog component:
Fix: Placeholder text indicates whether the component is meant for picking a date, a time, or both.
Previously, it always said
Select a date/time.Fix: atomic updates for date+time mode (do not update date when time selection is cancelled). In addition, chosen times are consistently displayed in a localized format, regardless of whether date and/or time selection is enabled.
Previously, when picking a date+time, only the time would be shown (localized). When picking just the date (canceling out of the time selection), the value saved would be the date + midnight, formatted as a raw ISO 8601 string (not localized). Now, when time selection is cancelled, we instead do not update the value at all. I believe this would be the least surprising behavior for most users.
before_picking_stuff.mp4
after_picking_stuff.mp4
Fix: When in date-only mode, only save the date (don't append midnight as the time)
before_picking_date.mp4
after_picking_date.mp4
This PR add tests coverage for these fixes as well as a bunch of other tests that were missing to begin with.
Pre-launch Checklist
///).