frizensami / nus-timetable-optimizer

Codebase for the NUS Timetable Optimizer, a tool to help students at the National University of Singapore optimize their timetables to their liking.
https://nus-optimizer.com
MIT License
19 stars 4 forks source link

Add autopopulate module via NUSMods share link #30

Closed benclmnt closed 3 years ago

benclmnt commented 3 years ago

Closes #28

frizensami commented 3 years ago

Oh, nice, thanks! Is the PR ready to review?

frizensami commented 3 years ago

The concept looks good, and thanks for the help with useEffect and the other housekeeping things.

A couple of high level points:

  1. I'm debating whether the share link population of modules should (a) add any modules that don't currently exist to the list (current behavior), or (b) we should delete the current list of modules and re-populate them with the link. I'm leaning towards (b)'s behavior being more expected for the user.

  2. For UI consistency, we can potentially do two things:

    • Can move the NUSMods share link area to within the Modules tab, either above or below the current manual selection boxes
    • Instead of an alert, we can reuse the same disappearing error message system to warn that the link format is invalid. Also I think the code continues even if the share link is invalid at the moment.

Let me know what you think!

frizensami commented 3 years ago

Also, could you "allow edits for maintainers" to the PR? I could do some of the small changes.

benclmnt commented 3 years ago

Re point 1, I think it is better to keep the current behaviour with an additional clear all modules button. So user can achieve b) by clearing all, then adding modules via link. If we were to implement b) directly, then we need to ask for confirmation that it will overwrite their current modules. I think the second one is much harder implementation wise. What do you think?

I agree with point 2. Let me push a fix.

I have allow edits for maintainers 😄

benclmnt commented 3 years ago

Oh, and I think another reason in favour of a) is to preserve the "make optional" and the lessonConstraints

frizensami commented 3 years ago

Okay, the clear all button + current behavior otherwise sounds like a good idea!

frizensami commented 3 years ago

Added a couple of small UI changes to make the choice between the link and adding manually more clear, let me know if that seems OK:

image

frizensami commented 3 years ago

I'll just fix the remove-all button for mobile layouts and then this should be good to go!

frizensami commented 3 years ago

Ah i spotted a bug where if the link contains an invalid module, e.g.

https://nusmods.com/timetable/sem-1/share?CS101S=REC:08A,TUT:09D,LEC:1&CS3230=LEC:1,TUT:12 (CS101S instead of CS1101S)

the optimizer gets into an infinite loop of making API calls. I'll also fix this one, probably a small issue.

frizensami commented 3 years ago

OK I fixed everything else, but the bug has something to do with the useEffect-triggered function to check if the new list of mods exists. Could you take a look @benclmnt?

Something to do with an infinite loop after the call for one of the mods returns false

    useEffect(() => {
        for (const module of modules) {
            if (!module.json) {
                // populate json field and then trigger a re-render
                populateModuleConstraintJsonField(module).then(_ => onModulesChange([...modules]));
            }
        }
    }, [modules]);
benclmnt commented 3 years ago

Pushed a fix + reduce number of re-renders :)

frizensami commented 3 years ago

I'll deploy this in the next couple of hours :)

frizensami commented 3 years ago

Deployed!