-
Notifications
You must be signed in to change notification settings - Fork 1
feat: days of week meetings now pull and display current dates #232
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
KevinWu098
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.
Just a couple of comments. Other than that, fix the merge conflict and we should be good to go
| const dateToDisplay = availabilityDates[selectedZotDateIndex].day; | ||
|
|
||
| const displayDate = isAnchorDateMeeting(meetingDates) | ||
| ? getCurrentWeekDateForAnchor(dateToDisplay) | ||
| : dateToDisplay; | ||
|
|
||
| const formattedDate = displayDate.toLocaleDateString("en-US", { |
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.
issue: I don't love that we pass in meetingData and duplicate a good deal of code in order to calculate just one date for display.
suggestion: Can we refactor this so that rather than duplicate code and pass around meetingData, we could calculate this in availability and pass that value down?
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.
Yup, I have just removed the duplicate code and put it under convertedDates in avaliability.tsx to replace meetingDates/Data as necessary
src/lib/types/chrono.ts
Outdated
| export function getDayOfWeekFromDateString(dateString: string): number { | ||
| return new Date(dateString).getDay(); | ||
| } |
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.
issue: is this method used?
|
Is this ready to re-review? How's fixing the merge conflict coming along? |
Yes, I have hopefully fixed the merge conflict as well as the issues presented in your comments. Please let me know if there is anything else I could improve/fix, thank you! |
KevinWu098
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.
LGTM! One minor nit and then we can merge
| [fromTimeMinutes, toTimeMinutes] | ||
| ); | ||
|
|
||
| const convertedDates = useMemo(() => { |
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.
nit: will be somewhat verbose, but how about anchorNormalizedDate? "converted" is ambiguous — converted from what? what was converted? etc
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.
Yup, I agree, I have changed all instances of convertedDates
| const { currentPage, itemsPerPage } = useAvailabilityPaginationStore(); | ||
|
|
||
| const { currentPage, itemsPerPage } = useAvailabilityPaginationStore( | ||
| useShallow((state) => ({ |
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.
good!
Summary
Video Demo
Screen.Recording.2026-01-05.at.9.05.53.PM.mov
Completes #221