nusmods icon indicating copy to clipboard operation
nusmods copied to clipboard

Custom modules

Open agnesnatasya opened this issue 4 years ago • 5 comments

Context

#2040
Feature request to add customised module to the timetable.

Implementation

I created a form that will require the user to input necessary information to create class of the customised module. The added class is supposed to appear in the timetable. demo feature

This is the responsive design for several screen widths

  • Wide screen Screenshot 2020-07-26 at 3 42 52 PM

  • Below md Screenshot 2020-07-26 at 3 43 03 PM

  • Small screen Screenshot 2020-07-26 at 3 43 15 PM

Other Information

This is my first PR, I encountered a problem but not really sure how to solve it. After the submit button is clicked, it dispatches the action, and in the log, the difference of the state is correct, but the page is just loading and it just hang in there. In the log, there is no log of waiting or doing for something, so I am not sure what did I do wrong.

If this problem has been rectified, I will modify the reducer, when it encounters the same module code and title, there are some checks that needs to be done.

Any help is appreciated, thank you!

agnesnatasya avatar Jul 26 '20 07:07 agnesnatasya

Codecov Report

Merging #2785 into master will decrease coverage by 0.60%. The diff coverage is 8.04%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2785      +/-   ##
==========================================
- Coverage   45.95%   45.34%   -0.61%     
==========================================
  Files         248      250       +2     
  Lines        5273     5359      +86     
  Branches     1222     1234      +12     
==========================================
+ Hits         2423     2430       +7     
- Misses       2850     2929      +79     
Impacted Files Coverage Δ
website/src/reducers/moduleBank.ts 30.76% <0.00%> (-1.67%) :arrow_down:
...ite/src/views/timetable/CustomModulesContainer.tsx 0.00% <0.00%> (ø)
website/src/views/timetable/TimetableContainer.tsx 67.24% <ø> (ø)
website/src/views/timetable/TimetableContent.tsx 0.00% <0.00%> (ø)
website/src/views/timetable/CustomModulesForm.tsx 4.25% <4.25%> (ø)
website/src/actions/timetables.ts 54.44% <4.34%> (-17.20%) :arrow_down:
website/src/reducers/timetables.ts 82.60% <100.00%> (+0.58%) :arrow_up:
website/src/types/modules.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update eb521e6...a32aace. Read the comment docs.

codecov[bot] avatar Jul 29 '20 12:07 codecov[bot]

Hi @ZhangYiJiang thank you so much for your help and review. I have solved the loading problem by making moduleBank reducer process ADD_CUSTOM_MODULE action too. I have also implemented all of your review suggestions.

The UI is responsive, here is the display in larger screen, you can look at the other screen widths in the deployment preview. Screenshot 2020-07-29 at 8 54 59 PM

This functionality is also available in the vertical mode. Screenshot 2020-07-29 at 9 58 39 PM

Several cases that I considered:

  • If the user adds a custom module with same code as one of their custom modules, and the title is different, a notification will be displayed.
  • If the user adds a custom module with the same code and title as one of their custom modules, the classes willl be added to the Semester data
  • All custom module code added will be appended with an apostrophe, to indicate that it is a custom module

Thank you!

agnesnatasya avatar Jul 29 '20 14:07 agnesnatasya

Using the module code field to indicate a module is custom is probably a bad idea. You're using a single field to store two pieces of unrelated information, and that's only going to cause issues in the future. A better way would be to add a customModule?: boolean property to the Module type and use that instead.

The UI is going to take a bit of work. You probably need a way for the user to specify different lesson types. The horizontal mode UI is also a bit too wide - you can probably fit the lesson fields on one line, which would make it easier to specify additional lessons. You'll also need to consider UI for editing existing custom modules and removing them.

ZhangYiJiang avatar Jul 31 '20 04:07 ZhangYiJiang

Hi @ZhangYiJiang thank you so much for your review again, appreciate it. The reason that I add apostrophe, ', in the code of the custom module, is to not confuse the user when they input the same code as a pre-existing module. For example, if someone puts CS1010 in the custom module, the name will be CS1010', so that it is still displayed as a different module from the original CS1010 module from NUSMods, so someone can have both in the timetable.

Also, it is to prevent custom module with the same code as a pre-existing module to overwrite the pre-existing module in the reducer. For example, in timetables reducer, if the moduleCode is the same, the old one will be overwritten, because it is a key-value pair in the dictionary.

    case ADD_MODULE:
    case ADD_CUSTOM_MODULE:
      return {
        ...state,
        [moduleCode]: action.payload.moduleLessonConfig,
      };

If I add a customModule? field in Module type, it still cannot handle this case because it is key-pair of dictionary.

May I clarify what do you mean by 'You probably need a way for the user to specify different lesson types'? There has been a 'lesson type' field in the form, or do I understand you wrongly?

Also, for the part 'you can probably fit the lesson fields on one line, which would make it easier to specify additional lessons', do you want the UI for horizontal mode on large screen or above, to be like

Module Code, Module Title and Module Credit in first line
Lesson Type, Class No, Venue, Day, Start Time, End Time in second line

?

Thank you very much!

agnesnatasya avatar Aug 09 '20 18:08 agnesnatasya