Some minor refactors #26
Conversation
| {schedulesToShow.map((schedule: Schedule, i: number) => ( | ||
| <div> | ||
| {schedulesToShow.map((schedule: Schedule, i) => ( | ||
| <div key={i}> |
There was a problem hiding this comment.
I think it's OK to use index as a key here because schedulesToShow never changes (we are not deleting or inserting anything in the array).
There was a problem hiding this comment.
Yup.
Nit: use s for index
| {LIMITS.map(([num, str]) => ( | ||
| <option value={num}>Generate ≤{str}</option> | ||
| {LIMITS.map(([num, str], idx) => ( | ||
| <option value={num} key={idx}> |
There was a problem hiding this comment.
Same reason as above.
Good work finding these missing keys via the console.
There was a problem hiding this comment.
I think it's OK here because LIMITS is a constant so we don't have to worry about the content of the array being changed.
There was a problem hiding this comment.
Immutable so yup.
Nit: Use the value (num) for the key.
There was a problem hiding this comment.
Really great work!
Make changes based on comments. Argue your position if you disagree.
I added a feature request in SectionPicker. You could add that to an issue (let me know if you can't create the issue) and assign yourself (make sure to label as "Enhancement").
I know there's a lot of comments and request for change this is fine work. I look forward to a revision.
Also, make sure to squash relevant commits together.
| {schedulesToShow.map((schedule: Schedule, i: number) => ( | ||
| <div> | ||
| {schedulesToShow.map((schedule: Schedule, i) => ( | ||
| <div key={i}> |
There was a problem hiding this comment.
Yup.
Nit: use s for index
| {LIMITS.map(([num, str]) => ( | ||
| <option value={num}>Generate ≤{str}</option> | ||
| {LIMITS.map(([num, str], idx) => ( | ||
| <option value={num} key={idx}> |
There was a problem hiding this comment.
Immutable so yup.
Nit: Use the value (num) for the key.
app/src/components/SectionPicker.tsx
Outdated
| }, | ||
| enabled: !!props.searchText, // Prevents query when searchText is empty | ||
| enabled: false, | ||
| // enabled: !!props.searchText, // Prevents query when searchText is empty |
There was a problem hiding this comment.
Remove commented code
app/src/components/SectionPicker.tsx
Outdated
| // isFetching | ||
| // ? props.soc instanceof SOC_API | ||
| // ? props.soc.searchCourses(searchBy, props.searchText) | ||
| // : [] |
There was a problem hiding this comment.
Remove commented code
| const { term, year } = SOC_Generic.decodeTermString( | ||
| t.CODE, | ||
| ); | ||
| return ( | ||
| <option value={t.CODE}> | ||
| <option value={t.CODE} key={idx}> |
|
I changed SectionDisplay to a functional component because using class component caused issues with the key prop not being unique for some reason. Functionality should remains the same. |
I added a button to fetch courses on click, instead of having it fetch every time the user types something and also refactored how the courses are display