Conversation
- Still needs to determine which modules are completed
|
Visit the preview URL for this PR (updated for commit 9d8e2b7): https://extendafamily-7613e--pr176-jimmy-finished-modul-f80genot.web.app (expires Wed, 14 Jan 2026 01:42:59 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: f8bb7b5cd838ab636347dc54fd0ab08ab3715d31 |
There was a problem hiding this comment.
Pull request overview
This PR implements a new Finished Modules page UI that allows learners to view their completed course modules. The implementation adds a new route, creates several new components for displaying finished modules in an accordion layout, and updates the home page to include navigation to this new feature. However, the PR description notes that logic to determine which modules are actually completed is still pending.
Key changes:
- New Finished Modules page with accordion-style unit sections and module grid display
- Navigation button added to home page for accessing the Finished Modules page
- Layout adjustments to home page components for responsive design with flexbox wrapping
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/constants/Routes.ts | Adds FINISHED_MODULES_PAGE route constant |
| frontend/src/App.tsx | Registers FinishedModules route with role-based access control |
| frontend/src/components/pages/Home.tsx | Updates layout with responsive flexbox and adds navigation button to Finished Modules page |
| frontend/src/components/pages/FinishedModules.tsx | Main page component that fetches and displays all units and modules with expand/collapse functionality |
| frontend/src/components/finished_modules/index.ts | Exports finished modules components |
| frontend/src/components/finished_modules/FinishedModulesContent.tsx | Content component that renders sorted unit sections with loading and error states |
| frontend/src/components/finished_modules/UnitSection.tsx | Accordion component for displaying a unit with its modules |
| frontend/src/components/finished_modules/ModulesGrid.tsx | Grid component that renders module cards using ModuleCardLearner |
| frontend/src/components/learners/NavButton.tsx | Reduces fixed width from 515px to 350px for better responsive layout |
| frontend/src/components/learners/FacilitatorCard.tsx | Changes width from fixed 1045px to maxWidth for responsive behavior |
| frontend/src/components/course_viewing/library-viewing/ModuleCardLearner.tsx | Adds "FINISHED ON [DATE]" badge with checkmark icon to all module cards |
| frontend/src/components/feedback/feedback-admin-view/FeedbackAdminView.tsx | Removes redundant comment |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Load all data on component mount | ||
| useEffect(() => { | ||
| const loadData = async () => { | ||
| setLoading(true); | ||
| setError(null); | ||
|
|
||
| const fetchedUnits = await fetchUnits(); | ||
|
|
||
| // Fetch modules for all units | ||
| await Promise.all( | ||
| fetchedUnits.map((unit) => fetchModulesForUnit(unit.id)), | ||
| ); | ||
|
|
||
| setLoading(false); | ||
| }; | ||
|
|
||
| loadData(); |
There was a problem hiding this comment.
The FinishedModules page currently displays all modules for all units without any filtering logic to show only completed modules. According to the PR description, "Still needs to determine which modules are completed." The page should either filter modules based on completion status or clearly indicate this is a work-in-progress that displays all modules temporarily.
| display="flex" | ||
| width="100%" | ||
| minHeight="100vh" | ||
| height="100vh" |
There was a problem hiding this comment.
Setting both height="100vh" and minHeight="100vh" is redundant. When height is set to a fixed value like 100vh, minHeight has no effect. Consider using only minHeight="100vh" to allow the container to grow if content exceeds viewport height, or only height="100vh" if a fixed height is truly required.
| height="100vh" |
| sx={{ | ||
| width: "43.192px", | ||
| height: "37.831px", | ||
| color: "#000000", |
There was a problem hiding this comment.
Hardcoded color value "#000000" should use the theme palette for consistency. Consider using theme.palette.text.primary or another appropriate theme color instead.
| color: "#000000", | |
| color: theme.palette.text.primary, |
| } | ||
| sx={{ | ||
| width: "100%", | ||
| backgroundColor: expanded ? "#F5F5F5" : "transparent", |
There was a problem hiding this comment.
Hardcoded color value "#F5F5F5" should use the theme palette for consistency. Consider using theme.palette.Neutral[100] or another appropriate theme color instead.
| backgroundColor: expanded ? "#F5F5F5" : "transparent", | |
| backgroundColor: expanded ? theme.palette.Neutral[100] : "transparent", |
| <Stack | ||
| direction="row" | ||
| padding="4px 8px" | ||
| justifyContent="center" | ||
| alignItems="center" | ||
| gap="8px" | ||
| borderRadius="4px" | ||
| bgcolor={theme.palette.Success.Light.Hover} | ||
| color={theme.palette.Success.Dark.Default} | ||
| > | ||
| <CheckCircle sx={{ fontSize: 15 }} /> | ||
| <Typography variant="labelMedium">FINISHED ON [DATE]</Typography> | ||
| </Stack> |
There was a problem hiding this comment.
The finished module badge is unconditionally rendered in the ModuleCardLearner component, which is used in multiple places including the regular course viewing grid (CourseModulesGrid.tsx). This will cause all modules to display the "FINISHED ON [DATE]" badge everywhere, not just on the Finished Modules page. The component needs a prop to conditionally show the badge, such as showFinishedBadge or isFinished, which should only be true when displaying actually finished modules.
| color={theme.palette.Success.Dark.Default} | ||
| > | ||
| <CheckCircle sx={{ fontSize: 15 }} /> | ||
| <Typography variant="labelMedium">FINISHED ON [DATE]</Typography> |
There was a problem hiding this comment.
The hardcoded placeholder text "[DATE]" should be replaced with actual completion date data. This appears to be incomplete functionality based on the PR description noting "Still needs to determine which modules are completed".
| <Typography variant="labelMedium">FINISHED ON [DATE]</Typography> | |
| <Typography variant="labelMedium">FINISHED</Typography> |
Notion ticket link
Ticket Name
Implementation description
Steps to test
What should reviewers focus on?
Checklist