Custom modules #2785
Custom modules #2785
Conversation
…custom modules when timetable is set
There was a problem hiding this comment.
Good job on your first PR!
The reason the page won't stop loading is because the final rendered timetable needs data from two places - the timetable config, which you have implemented, and the actual Module object loaded from the API and cached in module bank, which you have not. If the page finds a config but no module, it assumes the module is not loaded, and displays a spinner.
To fix this, you need to store the custom module's data in the module bank too. You will also need to persist this. I suggest adding a new customModules key to the settings reducer (which is already persisted) and creating a selector to merge that with the module bank.
Do also run the linter before pushing
| onChange={(e) => this.props.onChangeVenue(e)} | ||
| placeholder="Venue." | ||
| /> | ||
| <Downshift onChange={this.onSelectDay}>{this.renderDropdownDay}</Downshift> |
There was a problem hiding this comment.
These dropdown menu should just be <select> elements - use Downshift only if you want comboboxes where there are too many options to select without searching. See AvailabilitySearch for an example of time selection. You might actually be able to reuse that component if you can make it more generic
| $item-padding-vertical: 0.6rem; | ||
| $btn-margin: 0.5rem; | ||
|
|
||
| .formGroup { |
There was a problem hiding this comment.
Should use Bootstrap's form and layout system instead of trying to rebuild everything from scratch https://getbootstrap.com/docs/4.5/components/forms/
| <input | ||
| className={classnames(styles.input, styles.titleIcon)} | ||
| onChange={(e) => this.setState({ moduleCode: e.target.value })} | ||
| placeholder="Module Code" |
There was a problem hiding this comment.
Avoid using the placeholder for label. Each form elements should have a proper label element associated with them (which can be hidden visually if needed)
| }; | ||
|
|
||
| isFormValid = () => { | ||
| const validity = Boolean( |
There was a problem hiding this comment.
Try relying on HTML validation instead by adding required property to input elements
| timestamp: number; | ||
| }; | ||
|
|
||
| class CustomModulesForm extends React.Component<Props, State> { |
There was a problem hiding this comment.
If you can, try creating functional components. At least make this a PureComponent for better performance
…t existing modules
…ing module code and validate the title
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
|
Deployment preview for |
|
Hi @ZhangYiJiang thank you so much for your help and review. I have solved the loading problem by making The UI is responsive, here is the display in larger screen, you can look at the other screen widths in the deployment preview. This functionality is also available in the vertical mode. Several cases that I considered:
Thank you! |
| renderInputClassNo = () => { | ||
| return ( | ||
| <div className="form-group"> | ||
| <label htmlFor="module-code">Class No.</label> |
There was a problem hiding this comment.
IDs should be unique. module-code might be a bit generic, so you want to prefix it with something, and here it is a copy-paste error. Also class no. can probably be generated automatically - they don't matter for custom modules
There was a problem hiding this comment.
Okay, I will add '-custom-module' to the end of it to ensure uniqueness. For class number, I think it is still useful for custom modules, there might be several tutorial classes, for example.
|
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 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. |
|
Hi @ZhangYiJiang thank you so much for your review again, appreciate it. 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. If I add a 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 ? Thank you very much! |
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.
This is the responsive design for several screen widths
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!