-
Notifications
You must be signed in to change notification settings - Fork 11.6k
fix: Updated DateRangePicker in limit future booking #20483
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
Conversation
|
@21prnv is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
Graphite Automations"Add consumer team as reviewer" took an action on this PR • (04/01/25)1 reviewer was added to this PR based on Keith Williams's automation. "Add community label" took an action on this PR • (04/01/25)1 label was added to this PR based on Keith Williams's automation. "Add ready-for-e2e label" took an action on this PR • (04/01/25)1 label was added to this PR based on Keith Williams's automation. |
retrogtx
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.
the comments seem unnecessary, one summarized line or nothing at all will do
rest looks good to me 🙏
E2E results are ready! |
| // If both dates exist | ||
| if (date.getTime() === dates.startDate.getTime() && dates.endDate) { | ||
| onDatesChange({ startDate: date, endDate: date }); | ||
| } else if (date <= dates.endDate) { |
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.
With this logic we can only change the start date. We can't change the end date unless we pick a date past the current end date. Makes for a confusing UX.
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.
hey @shawnCaza thx for giving more clarity I'm working on this issue
|
I think we need state to keep track of the previous click. To handle the logic correctly we need to know if the last click set a start date or an end date. As a user in a date picker, my expectation is that my first date click will set the start date. The next date click should set the end date. Subsequent clicks should follow the same pattern. We only want to deviate from that in special circumstances, such as picking an end date that is before the start date. In any case where the start date is reset, the next click should set the end date IMO. If we close the date picker and re-open then then next click should be a 'start' click. |
|
Hey @shawnCaza I understand exactly what you're saying so for better UX the date picker should follow this flow.let me know if I'm wrong here!
|
|
@21prnv Yes, that's how I would do it. Others might have different opinions about 3. There's probably even clearer ways to do it that would require a more drastic ui change. Like choosing start and end date in a separate picker. But I think doing it as mentioned in 3 will provide a quick improvement over the existing UX. Of course, you should:
|
|
@shawnCaza fixed the issue please check Screen.Recording.2025-04-02.113111.mp4 |
|
@21prnv That looks like the same problem as before but in reverse. It's now hard to change the start date. You won't be able to solve this problem with that complicated nested if/else logic alone. I would suggest tracking the state of the previous date selection type like this: Then we can be clearer in the logic about what we want to do based on what happened with the previous date selection. If you like this idea you'd need to complete the missing parts in the second nested if statement. Note I took out the logic for handling the scenario where dates.startDate and Dates.endDate are undefined. I'm not sure in what scenario the values would come in undefined. Is there any problem in just setting default values if they are undefined? In the scenario where I make a new event. The range seems to default to current day for both start and end. |
|
hey @shawnCaza may be im wrong here but I think we don't need any extra hook here but still I can optimise that nested loops logic but before here the current code logic goes, in the previous video its just small recoding may be that's you didn't get the clarity:
I hope this clarifies things!) |
|
@21prnv I tested your branch locally. This is my experience:
The only way for me to change the start date, if the desired start date is between the current start date and end date, is to first change the end date to the new start date, then click that date again so end and start are the same, then re-select my end-date. I'm not saying my method with useState is perfect UX, but I think it's less confusing. |
|
With the useState approach it might even be more intuitive for the user if we handled the case where nothing has been clicked yet slightly differently |
|
A separate input box, that user can type in, for both start and end range, likely provides the simplest ux. |
|
Hey @shawnCaza , I tested your code and now understand everything. It has nothing to do with our code the issue is caused by that single date picker field. I tried my logic and changed the code multiple times, but the user can still move either the start date or the end date. I also tested your code, but in the end, the issue of changing the end date still persists. You're correct that we need two date pickers, but for that, we need to add another date picker, which requires raising a new issue and waiting for approval. That will take time. So, my suggestion is to add my logic for now and close this PR. |
|
@21prnv I'm not sure if we are seeing the same things when we are testing out these options or if we just have different preferences. In either case I don't feel I have anything new to add to explain my perspective. If you are confident with your PR you should stick with it for review. Thanks for looking into this with me and working on a solution. |
|
Hey @shawnCaza , thanks for communicating and elaborating on this. I would say that the single date picker is causing all the issues, but don't worry I will raise a new issue, and once it's approved, we will add the new end date picker. |
* fix: Updated DateRangePicker in limit future booking * removed unnecessary comments * minor changes in end date selection * final changes in end date selection * minor changes in end date selection * fixed issue of logs * added allowPastDates props for out of office data selection

What does this PR do?
Fixed date range selection to maintain end date when selecting earlier dates
Added toggle functionality: clicking start date again converts range to single-day range
Ensured single date selections automatically set both start and end dates to the same day
Updated range behavior: clicking start date in a range (e.g., 2-9) now sets it to single-day range (e.g., 2-2) instead of clearing end date ( it will fix the end date issue ex: start date comes after end date )
Fixes #20464
Visual Demo (For contributors especially)
Screen.Recording.2025-04-01.194432.mp4
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?