-
Notifications
You must be signed in to change notification settings - Fork 343
Add planner import and export #4077
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
Add planner import and export #4077
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@leslieyip02 is attempting to deploy a commit to the modsbot's projects Team on Vercel. A member of the Team first needs to authorize it. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4077 +/- ##
==========================================
- Coverage 54.52% 52.94% -1.58%
==========================================
Files 274 287 +13
Lines 6076 6612 +536
Branches 1455 1607 +152
==========================================
+ Hits 3313 3501 +188
- Misses 2763 3111 +348 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Cool feature!
I'm mostly okay with the code, but I have some comments about the overall UX of the feature.
Feel free to disagree with any of these, and if I agree on all the disagreements I'm happy to approve this.
-
I think there should be a warning dialog for the "Clear" button, instead of a tooltip. This would make it similar to how the "Reset" button works on the timetable. Also, perhaps using the same icon and text ("Reset") as the timetable would make more sense.
-
We can afford to be more descriptive on errors. I don't think we should display the gory syntax error, but perhaps something like "Something went wrong when trying to import the file. Try to re-download the planner file from the source!" could help non-tech-savvy users.
Finally, I'm not sure if it's a typo but the "closes #3483" seems to be pointing to an unrelated PR? Wrong number?
Oops, it should be closing #3843. |
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! speedy af
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Context
Closes #3843.
Since the new AY's modules are going to be added soon, the ability to import/export planners would be quite helpful for planning.
Course.Planner.-.NUSMods.Mozilla.Firefox.2025-06-27.23-59-32.mp4
The video looks a bit weird because my screen recorder hides my download window.
Implementation
.json
file of their current conifg.json
config to write over the current configzod
for schema validation (since it's used in the export service as well)