Repository navigation
Refactored react_modal.js.jsx into modular functional TypeScript comp… - #1822
Conversation
65acfc9 to
ad04b64
Compare
Coverage Report for CI Build 40797021318Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.2%) to 60.608%Details
Uncovered Changes
Coverage Regressions138 previously-covered lines in 5 files lost coverage.
Coverage Stats
💛 - Coveralls |
david-yz-liu
left a comment
david-yz-liu
left a comment
There was a problem hiding this comment.
Nice work, @danielrafailov! Sorry it took me a while to review this one. I left a few inline comments. In response to your comments/questions:
- Please make a new PR to give the Leaflet map modal rendering, and thanks for pointing this out
- Please go ahead in this PR and add JSDoc comments to
types.ts. That's a great contribution to the code quality and certainly consistent with the bulk of the work you're doing here already.
| @@ -0,0 +1,162 @@ | |||
| import { JSX, useRef } from "react" | |||
There was a problem hiding this comment.
For the JSX, use a type-only import (if it's only being used as a type annotation). This applies broadly across all files; please also review all imports to see if there are other type-only imports.
| export function Description({ course, sessions }: DescriptionProps): JSX.Element { | ||
| const timetableUnavailable = | ||
| Object.keys(sessions).length !== 0 && | ||
| sessions.F?.length === 0 && |
There was a problem hiding this comment.
This seems to change the explicit logic, as undefined === 0 is false.
I think we can instead check whether these lengths are falsy (0, undefined)
| //We want to use the Timetable component, but that component needs to be independent before using it here | ||
| return ( | ||
| <div> | ||
| {/* Description does not have a strong label prefix */} |
There was a problem hiding this comment.
I'm not sure what "strong label" means here. Also I would be explicit about why this matters, saying something like "and so is rendered separately"
| * Generate the data needed for the course modal table based on the meeting times corresponding | ||
| * to a course in a given session. | ||
| * @param {MeetingItem[]} allMeetTimes An array of MeetingData objects corresponding to a particular course. | ||
| * @param {string} session The session (F, S, Y) to query. |
There was a problem hiding this comment.
Please tighten this type constraint, and other session uses, to literal types: https://www.typescriptlang.org/docs/handbook/2/everyday-types.html#literal-types.
(I'm not sure how much of a cascade of issues this might create, so let me know if this seems unwieldy.)
|
|
||
| /** | ||
| * Manages state for currVisitedIndex when we click the forward button to | ||
| * gp to the next course in visitedCourses |
There was a problem hiding this comment.
There's a typo on the word "gp". But also the wording on this line should match the wording on the navegateBack docstring
david-yz-liu
left a comment
david-yz-liu
left a comment
There was a problem hiding this comment.
Nice work, @danielrafailov!
Proposed Changes
react_modal.js.jsxby converting legacy class-based components into modular functional components. I also converted the files from JavaScript to TypeScript as requested to catch bugs at compile time and encourage documentation.CampusMap,CourseModal,DayBox,Description,FocusModal,MapModal, andMapSidebarinto their own functional components, and created type interfaces for all of their props and state variables.ModalContent: Removed because it was simply a wrapper forDescription, whichCourseModalwas already rendering.VideoclassfocusInfostate variableformatCourseNamehelper functionType of Change
Checklist
Before opening your pull request:
After opening your pull request:
Questions and Comments
CampusMap.tsx, I noticed a bug where the Leaflet map does not render when opening the Campus Map modal from the timetable view. I asked gemini about this and it said that this is a known CSS issue where the Leaflet map container defaults to a height of0px(or does not render properly until resized).types.tsto document what each type represents and where it is used across the components. Let me know what you think of this.