Form for creating courses #149
Conversation
There was a problem hiding this comment.
Do we really need a whole page for this? I feel like this can be a component that can be used in an overview page of teachers. @Vucis what do you think?
|
|
||
| const { t } = useTranslation('translation', { keyPrefix: 'courseForm' }); | ||
|
|
||
| const apiHost = import.meta.env.VITE_API_HOST; |
There was a problem hiding this comment.
Move this outside the function, everytime a state changes this will re-import the env variable. Alternatively you can wrap it in an effect with an empty dependency array
| "courseName": "Course Name", | ||
| "submit": "Submit", | ||
| "emptyCourseName": "Course name should not be empty" |
There was a problem hiding this comment.
Add something like Message to the key, it's not obvious by the key what this stands for
| } | ||
|
|
||
| const data = { name: courseName }; | ||
| call_to_api(apiHost + '/courses', JSON.stringify(data), 'POST'); |
There was a problem hiding this comment.
Use backtick notation instead of + string
| * @param data - data to send | ||
| * @param method - POST, PATCH, GET or DELETE | ||
| */ | ||
| function call_to_api(path: string, data: string, method:string){ |
There was a problem hiding this comment.
Javascript/TypeScript uses camelNotation
| } | ||
|
|
||
| /** | ||
| * Helper function to send requests to the a</Button>pi |
| <Button | ||
| type="submit" | ||
| style={{ | ||
| position: 'absolute', |
There was a problem hiding this comment.
Why does the position need to be absolute? This can mess up lots of things when changing screen resolution
| }) | ||
| .then(response => response.json()) | ||
| .then(data => { | ||
| window.location.href = data.url; // navigate to data.url |
There was a problem hiding this comment.
Use appropriate functions for this, e.g. navigate
| method: method, | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| 'Authorization': 'teacher1' |
|
Yes, it might be better as a popup when a teacher clicks a button in their main course page, considering we only need the name of the course |
Fixes #157