Skip to content

Refactored react_modal.js.jsx into modular functional TypeScript comp… - #1822

Open
danielrafailov wants to merge 2 commits into
Courseography:masterfrom
danielrafailov:refactor-react-modal
Open

danielrafailov wants to merge 2 commits into
Courseography:masterfrom
danielrafailov:refactor-react-modal

Conversation

@danielrafailov

@danielrafailov danielrafailov commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

Proposed Changes

  • As per the PR title, I refactored react_modal.js.jsx by 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.
  • Specifically, I factored out CampusMap, CourseModal, DayBox, Description, FocusModal, MapModal, and MapSidebar into their own functional components, and created type interfaces for all of their props and state variables.
  • Dead code removal: I know we already discussed this, but as a reminder I have highlighted removed code and why it was removed below:
    • ModalContent: Removed because it was simply a wrapper for Description, which CourseModal was already rendering.
    • Unused references removed:
      • Video class
      • focusInfo state variable
      • formatCourseName helper function

Type of Change

Type Applies?
🚨 Breaking change (fix or feature that would cause existing functionality to change)
✨ New feature (non-breaking change that adds functionality)
🐛 Bug fix (non-breaking change that fixes an issue)
🎨 User interface change (change to user interface; provide screenshots)
♻️ Refactoring (internal change to codebase, without changing functionality) X
🚦 Test update (change that only adds or modifies tests)
📦 Dependency update (change that updates a dependency) X
🔧 Internal (change that only affects developers or continuous integration)

Checklist

Before opening your pull request:

  • I have performed a self-review of my changes.
    • Check that all changed files included in this pull request are intentional changes.
    • Check that all changes are relevant to the purpose of this pull request, as described above.
  • I have added tests for my changes, if applicable.
    • This is required for all bug fixes and new features.
  • I have updated the project documentation, if applicable.
    • This is required for new features.
  • If this is my first contribution, I have added myself to the list of contributors.
  • I have updated the project Changelog (this is required for all changes).

After opening your pull request:

  • I have verified that the CI checks have passed.
  • I have reviewed the test coverage changes reported by Coveralls.
  • I have requested a review from a project maintainer.

Questions and Comments

  • While refactoring 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 of 0px (or does not render properly until resized).
  • To keep this PR focused strictly on the TypeScript and functional component refactoring, I left existing styling as-is.
  • Could I take this on as a follow-up task/issue once this PR is reviewed? I'd be happy to open a dedicated PR to fix the container sizing.
  • Additionally, I was thinking about adding JSDoc comments to types.ts to document what each type represents and where it is used across the components. Let me know what you think of this.

@coveralls

coveralls commented Oct 24, 2026 •

Copy link
Copy Markdown

Coverage Report for CI Build 40124537948

Coverage increased (+0.3%) to 60.638%

Details

  • Coverage increased (+0.3%) from the base build.
  • Patch coverage: 106 uncovered changes across 6 files (102 of 208 lines covered, 49.04%).
  • No coverage regressions found.

Uncovered Changes

File Changed Covered %
js/components/common/MapModal.tsx 58 15 25.86%
js/components/common/CampusMap.tsx 32 0 0.0%
js/components/common/DayBox.tsx 15 0 0.0%
js/components/common/FocusModal.tsx 13 7 53.85%
js/components/common/MapSidebar.tsx 6 0 0.0%
js/components/common/utils.ts 4 0 0.0%
Total (8 files) 208 102 49.04%

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 4313
Covered Lines: 2679
Line Coverage: 62.11%
Relevant Branches: 1012
Covered Branches: 550
Branch Coverage: 54.35%
Branches in Coverage %: Yes
Coverage Strength: 159.7 hits per line

💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants