-
Notifications
You must be signed in to change notification settings - Fork 38
Date range in DatePicker #148
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
- Add DatePicker variants - Draft implementation DateRange for DatePicker - Extend DateRange struct - Use min and max dates in props as DateRange - Spell check fix for Calendar
- Fix date range selection - Support disabled ranges
|
Preview available at https://dioxuslabs.github.io/components/pr-preview/pr-148/ |
|
@ealmloff Finally ready to give my brainchild for review |
ealmloff
left a comment
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! I noticed a few bugs with the styling and logic of the calendar. We need some more playwright test (and potentially some inline tests) to verify the behavior of the new multi-calendar view and hopefully catch issues like this in the future.
There are two issues with the styling:
- The alignment between the days of the week and the calendar grid is off
- The heading is a bit cluttered. We can support the more complex dropdown month/year, but the simple variant would look nicer in the demo. We also only need the month change buttons on the outside of calendar group instead of between each calendar
| /// DatePickerCalendar { | ||
| /// selected_date: selected_date(), | ||
| /// on_date_change: move |date| selected_date.set(date), | ||
| /// calendar: Calendar, |
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.
Is the change in this example required to make the example compile? We should have a default calendar and still allow you to pass in a selected_date and on_date_change callback
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.
Since the Date picker and Calendar are linked, the logic is designed so that passing params selected_date and on_date_change to the date picker component will automatically reassign them to the Calendar.
Or is there a need to allow the user to set these properties separately for the Calendar?
In fact, on_date_change method for the Calendar is where these components synchronize their data.
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.
I thought this was the regular calendar. Not accepting selected date separately is good for the DatePickerCalendar, but it would still be nice to have a default calendar
a81221e to
ecbd1ce
Compare
|
@ealmloff I've fixed the style errors and various logical inaccuracies you mentioned above. And I have a question about the Calendar test error in Playwright.
|
- Fix docs - Add alignment between the days of the week and the Calendar grid - Add simple title variant in Calendar demo - Add css for Calendar title - Update time crate version (support PartialOrd for Month) - Fix view day on next/previous button click - Fix DateRangePickerContext (sync data) - Fix arrow navigation



Plan
Fixes #132