-
Notifications
You must be signed in to change notification settings - Fork 9
#3818 availability changes #3849
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
base: feature/calendar-improvements
Are you sure you want to change the base?
Conversation
JoshuaGoldberg
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.
This looks great! we discussed some changes already, and the routes have already been fixed, but I would just double check to make sure the available members panel doesn't cause the availability view to shift up and down when selecting/deselecting timeslots. Also, there seems to be something slightly off with how view availability selects the week (edit works fine), so that likely needs to be tweaked. Other than that, the main thing would be to port over the frontend design to switch from grid to table, as that would make it much nicer and not deprecated.
| include: { | ||
| members: getUserQueryArgs(organizationId), | ||
| leads: getUserQueryArgs(organizationId), | ||
| head: getUserQueryArgs(organizationId) |
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.
When getting an event, the default definitely shouldn't include the team's members. It probably also shouldn't include the members. These should be in a seperate query args that are used only for the specific endpoints that need it
|
|
||
| useEffect(() => { | ||
| if (open) { | ||
| if (open && !hasInitialized.current) { |
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 think everything in this if branch can just be put in the regular component with if (open) {...}, and then you don;t need the hasInitialized stuff
| // Reset the ref when modal closes | ||
| if (!open) { | ||
| hasInitialized.current = false; | ||
| } |
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 think this is irrelevant because when open changes the component will re-render and hasInitialized should be reset to false, but also I really don't like this hasInitialized approach because react re-renders are weird and we should avoid depending on them
Changes
Made the edit button open the edit modal, and the view availability button combine editing your availability and viewing everyone's availability
Screenshots
Checklist
It can be helpful to check the
ChecksandFiles changedtabs.Please review the contributor guide and reach out to your Tech Lead if anything is unclear.
Please request reviewers and ping on slack only after you've gone through this whole checklist.
yarn.lockchanges (unless dependencies have changed)Closes #3818