-
Notifications
You must be signed in to change notification settings - Fork 0
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
Implement monthly view calendar for admin scheduling UI #207
Conversation
Visit the preview URL for this PR (updated for commit 939d052): https://sistering-dev--pr207-lambert-shift-calend-jba7ndj6.web.app (expires Thu, 07 Apr 2022 22:18:20 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
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.
✨ Looks amazing! ✨
Nothing too big from me, just some questions and suggestions :)
}} | ||
fixedWeekCount={false} | ||
headerToolbar={false} | ||
initialDate={events.length > 0 ? events[0].start : new 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.
Something I discovered from the shift calendar is that FullCalendar does not re-render when initialDate
changes. Can you confirm if the initialDate
is updated when a new set of events is passed as props?
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.
Not quite sure how to test this without integrating the calendar into the header. I think maybe we can investigate into this in the calendar header ticket (where the events are being passed in).
What I suspect though, is that FullCalendar is not refreshing when the events props change. We might be able to get around this using a force refetch - https://fullcalendar.io/docs/Calendar-refetchEvents.
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.
Sounds good 👍
Was thinking of making wrapper like the one described in fullcalendar/fullcalendar#4684 (comment) for the week view calendar, would be applicable here too.
a0aeaac
to
fe11518
Compare
I left the Calendar component in Default.tsx so that you can take a look again when you review. I'll remove it once this PR is approved! |
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, will approve after addressing tiny lint issue
frontend/src/types/CalendarTypes.ts
Outdated
|
||
export type MonthEvent = Event & { | ||
groupId: string; | ||
}; |
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: newline please
072b67b
to
ed4917d
Compare
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.
🚢🚢🚢
Ticket link
Closes #177
Implementation description
Steps to test
Default.tsx
, importing components/variables when necessary:4. Feel free to change the test case, as long as all events of
ADMIN_SHIFT_CALENDAR_TEST_EVENTS
are in the same month.What should reviewers focus on?
Checklist