-
Notifications
You must be signed in to change notification settings - Fork 8.5k
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
fix: #15833 [CAL-4055] Same first slot in different timezones #15840
base: main
Are you sure you want to change the base?
Conversation
@vijayraghav-io 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 community label" took an action on this PR • (07/19/24)1 label was added to this PR based on Keith Williams's automation. "Add consumer team as reviewer" took an action on this PR • (07/19/24)1 reviewer was added to this PR based on Keith Williams's automation. |
const includedDates = getAvailableDatesInMonth({ | ||
browsingDate: browsingDate.toDate(), | ||
minDate, | ||
minDate: getTodaysDateInTimeZone(timezone), |
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, instead of setting the minimum date here, can we modify or pass it from the Datepicker? This way, we eliminate the need for an extra prop like timezone and also minDate prop used.
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.
As explained below anyway we will have to pass one of the prop, if its minDate, we will have to pass from DatePickerComponent which is in packages/features/bookings/Booker/Components/DatePicker.tsx.
FYR, there are two different DatePicker.tsx files.
So this should be ok.
@@ -414,6 +423,7 @@ const DatePicker = ({ | |||
slots={!isBookingInPast ? slots : {}} | |||
includedDates={!isBookingInPast ? includedDates : []} | |||
isBookingInPast={isBookingInPast} | |||
timezone={timezone} |
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.
Instead of passing the timezone, can we set or modify the minDate from here?
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.
Explained, refer other comments.
@@ -38,6 +39,7 @@ export const DatePicker = ({ | |||
shallow | |||
); | |||
const nonEmptyScheduleDays = useNonEmptyScheduleDays(schedule?.data?.slots); | |||
const { timezone } = useTimePreferences(); |
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 @vijayraghav-io, can we do the same directly inside the DatePickerComponent
instead of passing it from here?
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.
Yes, i started with having useTimePreferences() inside <Days component, everything works fine w.r.t this fix and functionality.
But when i ran the complete tests, the test "date-override-list.test.tsx" fails without even showing proper error message.
I found it in really hard way spending so much time, then finally found that - useTimePreferences() inside Days() component or even inside getTodaysDateInTimeZone.ts was causing this test to fail.
components/DatePicker.tsx is the nearest parent component which was not throwing this test fail.
// 'todayDateInPreferredTZ' is dayjs date, we need to convert this into type 'Date', | ||
// as input required for getAvailableDatesInMonth() is 'Date' type. | ||
const offsetMinutes = dayjs().utcOffset(); | ||
const jsDate = new Date(`${todayDateInPreferredTZ.format("YYYY-MM-DDTHH:mm:ss")}Z`); // UTC format |
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.
Instead of jsDate
can we set localDate
or currentDate
?
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 am intending to be Date of type Date (JS/TS native object), as we can also get date from Dayjs().toDate() but this will again give date in local timezone
// Example : | ||
// Consider browser current date-time is - "Sat Jul 20 2024 01:00:00 AM (Asia/Kolkata GMT +5:30)". | ||
// And, if preferred timezone is "Pacific/Pago GMT -11:00", we need to get today's date as "Fri Jul 19". | ||
export function getTodaysDateInTimeZone(selectedTimeZone: string | undefined): Date { |
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.
selectedTimeZone
must not be undefined
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 should not be an issue, because always there will be a timezone selection in booking page and also there is a default value provided here
timezone: localStorage.getItem(timezoneLocalStorageKey) || dayjs.tz.guess() || "Europe/London", |
| undefined was added for typechecks ,because DatePicker prop should have timezone as optional , to be compatible with dependent components
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.
Can we add a unit test for this?
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.
@vijayraghav-io great works. Left some comment. Let me know what you think?
Yes i am adding |
Thankyou! replied inline |
Scenario of first day in month: https://www.loom.com/share/f917aa77fc4f442aadb7d31b94be7435?sid=0a61481c-d67b-4154-b68f-66ee6a19c490 |
@emrysal , can you please share your comments/reviews |
reminding to please review or approve if reviewed |
Co-authored-by: Carina Wollendorfer <[email protected]>
Thankyou! 🙏 , Sure have fixed the dot to show as per selected timezone's 'today' and verified. |
This PR is being marked as stale due to inactivity. |
Gentle Reminder! |
E2E results are ready! |
// 'todayDateInPreferredTZ' is dayjs date, we need to convert this into type 'Date', | ||
// as input required for getAvailableDatesInMonth() is 'Date' type. |
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.
Could you pls remove this extra 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.
reduced the overall comments
// with new Date(), we get browser's current date. | ||
// The below function gets today's date w.r.t to selected timezone. | ||
// Example : | ||
// Consider browser current date-time is - "Sat Jul 20 2024 01:00:00 AM (Asia/Kolkata GMT +5:30)". | ||
// And, if preferred timezone is "Pacific/Pago GMT -11:00", we need to get today's date as "Fri Jul 19". |
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.
same
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.
reduced to 1-2 lines, as i feel it's important and also for someone who is sees/reviews this code for first time.
The question may come for anybody seeing this for first time, why not use already existing function to get current date in timezone.
That's why had added detailed explanation, now reduced to just 2 lines.
// We need to set minDate to today's (or current) date w.r.t selected timezone. | ||
// If we just do new Date(), we get browser's current date. | ||
// getTodaysDateInTimeZone() function gets today's date w.r.t to selected timezone. | ||
// Returned Date will be of type Date (native TS/JS Date type). | ||
// As getAvailableDatesInMonth() function requires minDate in TS/JS Date type. |
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.
No need to explain everything in detail in a comment; just add important points if necessary. Instead try to self-review the code with explanations.
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.
removed this and added as self review comment
@@ -309,6 +321,8 @@ const DatePicker = ({ | |||
}; | |||
scrollToTimeSlots?: () => void; | |||
}) => { | |||
const minDate = getTodaysDateInTimeZone(selectedTimeZone); |
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.
We need to set minDate to today's (or current) date w.r.t selected timezone.
If we just do new Date(), we get browser's current date.
getTodaysDateInTimeZone() function gets today's date w.r.t to selected timezone.
Returned Date will be of type Date (native TS/JS Date type).
As getAvailableDatesInMonth() function requires minDate in TS/JS Date type.
@anikdhabal , have addressed your latest comments. |
…dateBookingRefs_credCreate
What does this PR do?
After Changes -
https://www.loom.com/share/d3e067e558184cd2aa7e62ca2e48c770?sid=76ab6332-5e38-4421-8247-e76991fa4751
Root cause -
The minDate, used to set the start date in getAvailableDatesInMonth was using browserDate, updated this to calculate today's date in selected timezone.
cal.com/packages/features/calendars/lib/getAvailableDatesInMonth.ts
Line 10 in a74522a
Mandatory Tasks (DO NOT REMOVE)