DateRangePicker: add ability for predefined ranges #608
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
hoorayimhelping
force-pushed
the
bucky-daterangepicker-predefined
branch
from
8dab553 to
4b19865
Compare
|
Hey @hoorayimhelping , sorry for the late reply. I was wondering, could we do this instead? I realize you may need to make more than a simple change, but it would look better I think 🙂 
|
hoorayimhelping
force-pushed
the
bucky-daterangepicker-predefined
branch
from
4b19865 to
485cf73
Compare
hoorayimhelping
force-pushed
the
bucky-daterangepicker-predefined
branch
from
485cf73 to
e729692
Compare
hoorayimhelping
force-pushed
the
bucky-daterangepicker-predefined
branch
from
e729692 to
93e7eae
Compare
| for (let i = 0; i < Math.abs(numberOfMonths); i++) { | ||
| const date = now.subtract(i, "month"); | ||
| if (date.date() === 1 && date.month() === now.month()) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
something is off here
“last 6 months” actually displays last 5 months:
It might be just the naming of the story. But I feel like passing "-6" into the helper should return 6 items.
We either want to iterate until <= for past months, or not exclude the current month
it's actually an interesting question. But it seems to me that including the current month is the more expected option. Think “Show last 6 months of billing data” — I would expect to see whatever part of the current month has already passed as well
There was a problem hiding this comment.
But it seems to me that including the current month is the more expected option. Think “Show last 6 months of billing data” — I would expect to see whatever part of the current month has already passed as well
found a potential conflict: pasing -1 will return the same result as passing 1 then
(the same edge case also applies to the current imnplementation: passing -1 would return an empty result, which is weird)
so I'm changing my mind
I feel like
- getPredefinedMonthsByNumber(1) should return the next month
- getPredefinedMonthsByNumber(-1) should return the previous month
- getPredefinedMonthsByNumber(0) should return nothing
- getPredefinedMonthsByNumber should accept a 2nd argument with configuration, that should have a
includeCurrentMonth: boolean- it can be true by default, then getPredefinedMonthsByNumber(0) will return the current month
There was a problem hiding this comment.
found a potential conflict: pasing -1 will return the same result as passing 1 then
Yeah, these are good points. I need to think over this a little deeper
something is off here “last 6 months” actually displays last 5 months...
That is an edge case around the first day of the month to avoid having a range where the start date is the same as the end date, namely the first day of the month (Aug 01, 2025 - Aug 01, 2025)
There was a problem hiding this comment.
That is an edge case around the first day of the month to avoid having a range where the start date is the same as the end date, namely the first day of the month (Aug 01, 2025 - Aug 01, 2025)
oh, I just realized it works like that
A few thoughts. And I found a bug.
- if it's a single day, can't we display it simply as
Aug 01, 2025? - We have props
futureDatesDisabledandfutureStartDatesDisabled- first the bug: you can still select future dates from the predefined list with these props set to true (which you shouldn't)
- then the thought: I feel like these props should affect how
getPredefinedMonthsByNumberbehaves. It should always return the whole month with them being false.
There was a problem hiding this comment.
Same with maxRangeLength, predefined dates can exceed this range, and still be selected
I guess it doesn't make sense to have predefined dates that don't match maxRangeLength at the same component, but we don't even warn about it
| ); | ||
| })} | ||
| </ScrollableContainer> | ||
| <StyledDropdownItem onClick={handleCustomTimePeriodClick}> |
There was a problem hiding this comment.
can we show the calendar on hover?
less clicks
plus in general, I feel like I expect nested items in a dropdown to appear on hover rather than on click
cc @crisalbu
| )} | ||
| </PredefinedCalendarContainer> | ||
| ) : ( | ||
| <CalendarRenderer calendarOptions={calendarOptions}> |
There was a problem hiding this comment.
nitpick: this can be a variable, perhaps in a useMemo(), to avoid repeating all the props in two places
ariser
dismissed
their stale review
Going on PTO next week, so I'll dismiss my request to unblock you
hoorayimhelping
force-pushed
the
bucky-daterangepicker-predefined
branch
from
93e7eae to
be605b8
Compare
Adds predefined ranges to
DateRangePicker.maxRangeLengthfutureStartDatesDisabled, to disable start dates in the futurepredefined dates list
limit of 15 for
dateRangeLimit