-
Notifications
You must be signed in to change notification settings - Fork 32
Date Picker component #121
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
base: main
Are you sure you want to change the base?
Conversation
- Select single day - Use time create for dates - Displaying a single month calendar view - No examples in docs - Fix CalendarSelectYear/CalendarSelectMonth date math bug
@ealmloff before implementing extended functionality and making the component look good, I'd like to get some feedback on the component's current architecture. The behavior with hiding the calendar may look suspicious, but for now I need this implementation to simplify debugging. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! This is one of the first components that builds on top of the existing primitives. You are re-using the calendar logic which is great, I wonder if we could also re-use the logic for the popover component
6b85c64
to
aba0045
Compare
- Add popover with Calendar - Pass Calendar as child - Correct input validation - Split from preview - Fix add Calendar empty cells - Localization
@ealmloff I'd like to request a component review again Important changes:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The interaction model seems a bit odd in the preview. I click on the dropdown to select a date, then it closes the preview with the range only half filled. Then if I click on the dropdown again, it lets me choose the second date and then the first date again before closing the preview.
It might make sense to remove range support from the date picker until the calendar component itself supports ranges
@npatsakula and I discussed the task and agreed that it needed to be broken down into subtasks, meaning we'd implement only one date selection within this wish. To support date ranges in the Сalendar and Date Picker, I created new tasks and changed the current task description with a note about the work plan change. |
open: open(), | ||
on_open_change: move |v| open.set(v), | ||
PopoverTrigger { attributes: props.attributes, | ||
svg { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This SVG is related to how the date picker is displayed, not a core part of the interactions / accessibility so it should also be in the styled version of the component in the preview
Plan
Fixes #119
Calendar
Fixes #129 Fixes #130
It was decided to break the task down into its subtasks.
A task was created to select a date range and implement a two-month view: supports displaying two consecutive months side-by-side #132