random course picker button #3735
Conversation
added a random course picker button in settings (first good issue)
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@Nimastic is attempting to deploy a commit to the modsbot's projects Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Thank you very much for the PR! We're glad to be the first open-source project you're contributing to :)
I've tested this locally and can confirm it works.
I think that placing the button in the settings is a great choice! Would it be possible to shift it above the "Privacy" section?
Unfortunately, I'll have to request some changes, on the grounds of code convention. See below for comments.
| { | ||
| "name": "nusmods", | ||
| "lockfileVersion": 3, | ||
| "requires": true, | ||
| "packages": {} | ||
| } |
There was a problem hiding this comment.
Could you remove package-lock.json and yarn.lock?
They both seem like artefacts from an accidental yarn install and npm install from the root of the repository.
| export const fetchModules = async () => { | ||
| try { | ||
| const response = await axios.get(`${BASE_URL}/moduleList.json`); | ||
| return response.data; | ||
| } catch (error) { | ||
| console.error('Error fetching modules:', error); | ||
| return []; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Can I check if there's a reason you added fetchModules to explicitly fetch all modules?
The module list should be available in Redux store, see website/src/types/state.ts for the structure of the store.
| } | ||
| const randomIndex = Math.floor(Math.random() * courses.length); | ||
| const randomCourse = courses[randomIndex]; | ||
| console.log('Random Course:', randomCourse); |
There was a problem hiding this comment.
We generally don't want to leave console.logs in production code. I think in this case, they aren't 100% necessary for the feature to work.
There was a problem hiding this comment.
There's one higher up as well (oops)
| const handleRandomCourseClick = async () => { | ||
| const randomCourse = await getRandomCourse(); // Fetch random course from API | ||
| if (randomCourse && randomCourse.moduleCode) { | ||
| history.push(`/courses/${randomCourse.moduleCode}`); |
There was a problem hiding this comment.
Could you "generate" the route to push using modulePage from website/src/views/routes/paths.ts?
|
Hi, if this issue is not taken care of, I can help to settle this PR :] |
Hi I added a feature request: random course picker button in settings (first good issue)
Context
#3671
Feature Request: Random Course Picker
Implementation
I created api.ts in website/src/utils/api.ts to fetch list of modules from NUSMods API
I also created randomCoursePicker.ts in website/src/utils/randomCoursePicker.ts to fetch a random course from the list of modules fetched from the API
Lastly I added button and function in website/src/views/settings/SettingsContainer.tsx for picking a random course
Overall, I created a random course picker button in the settings
Other Information
Hi I'm new to this as both a contributor on this repository and as an OS contributor. Do let me know if I have done anything wrongly.
I wasn't too sure where to add the button, hence i added it into settings. Do let me know if it isn't the right place to add it.
Hope to learn more :)