-
Notifications
You must be signed in to change notification settings - Fork 90
Schedule Link Sharing #1357
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: main
Are you sure you want to change the base?
Schedule Link Sharing #1357
Conversation
apps/antalmanac/src/components/Calendar/Toolbar/ScheduleSelect/ScheduleSelect.tsx
Outdated
Show resolved
Hide resolved
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.
I haven't read the code deeply, but I'll share a few thoughts/feedback before doing a deeper read later
- Feature is definitely cool
- the mergeShortCourseSchedules breaks because it adds a
(1)so adding a shared schedule "Foo" becomes "Foo (1)" if the receiving user has another schedule named "Foo" - Anytime we ask someone to sign in, we should make it easy for them — give them a button
- We should be more clear that this is a static copy
- Relating to #4... do we have to be a static copy? 🌚
|
Created new and separate Shared Schedule page
|
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 hate to be that guy but is there a way for you to consolidate the git diff so it only shows the file changes you made. If it's too much of a hassle it's whatevs we'll figure it out.
-
Also can you restore the merge conflicts (resolving them might resolve the git diff issue) and may fix the staging deployment which seems to be breaking
|
holay molay my bad |
a2d14ee to
b1ae03e
Compare
|
With some cursor shenanigans, I think I was able to fix it Also when are we playing aram |
|
Resolve merge conflict so the deployment can run, gracias |
|
I finally figured out what was causing all tests to not run, it has been fixed and staging works 🫶 |
alexespejo
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.
Haven't looked at code too indepth yet but some bugs 🐞:
- Unsaved changes
Repro: Opening a shared link -> editing the schedule (removing/adding classes) -> "refresh" -> "accept unsaved changes" -> the changes persist
The user either should be locked out of changing the shared schedule or lock unsaved changes from triggering
-
Design
- Small detail I think it would be nice to preview the schedule name in the drop down button so that there isn't just some empty space
- Maybe bold the name of the schedule being imported (e.g. "Viewing Shared Schedule: 2026 Winter)
- Sharing a deleted schedule id "breaks" the user's data
Repro: have two users open (1 and 2) -> (1) copy share link -> (1) edit and save the schedule -> (2) open the shared link -> recieve error message -> hit "go home" -> the user's data should appear empty
Refreshing the page will fix the schedule but it would be offputting the the user to think their data has been wiped
Summary
Implemented Schedule Link Sharing
Test Plan
Issues
How should I handle the multiple snackbars? Just one after everything loads or keep two to indicate when account schedules have loaded and when shared schedule has loaded? since they're separate things
Closes #1346