-
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 admin "select shift times" calendar #150
Conversation
Visit the preview URL for this PR (updated for commit 959dce8): https://sistering-dev--pr150-lambert-implement-ca-bcy6p2av.web.app (expires Sun, 13 Mar 2022 06:23:07 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
newEvents.splice(i, 1); | ||
break; | ||
} | ||
} |
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.
optionally you could use newEvents.indexOf()
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.
Looking good, thanks for fiddling around with CSS to get the correct styling! One main thing is to change events
to a prop, the rest are just small nits.
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 tested it out and the calendar looks sooo good! 🤩
a2ffc23
to
c92206d
Compare
Fixed up the details! Please let me know if there's anything else I can clean up 🛁 |
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.
UI looks great but we do need to rework the state management a bit. Again, sorry for not catching this earlier and let me know if you have any questions!
}; | ||
|
||
const ShiftCalendar = ({ shifts }: ShiftCalendarProps): React.ReactElement => { | ||
const [events, setEvents] = useState<Event[]>(shifts); |
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 lift the state up to the parent component (in this case it's CreatePostingShiftsPage
and eventually it'll be CreatePostingShifts
from #164 when merged). This is so that the create shifts component can update the posting context (global state) with shifts data along with other posting fields.
Sorry, I didn't completely think this through when I made my previous comment, but the change is a bit more involved than just adding a shifts
prop. We need the parent component to maintain the events
state and all functions for manipulating state (e.g. addEvent
, changeEvent
, deleteEvent
) should be passed in as props (and defined in the parent). The event count state should similarly be maintained in the parent and a function for getting the next event ID should also be provided as a prop.
Let me know if any further clarification is needed!
76ebc9f
to
1b7f90f
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.
Awesome stuff! 🚀 Go ahead and merge if you need to unblock yourself, though there is a one-liner change needed
Co-authored-by: Sherry Li <sherryhli@users.noreply.github.com>
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 #102
Implementation description
Steps to test
http://localhost:3000/admin/posting/create/shifts
and play around with the calendar!console.log(events)
in the code to check the output dataWhat should reviewers focus on?
Checklist