Conversation
|
Visit the preview URL for this PR (updated for commit ede8166): https://extendafamily-7613e--pr165-jimmy-activity-learn-f4837rtu.web.app (expires Mon, 15 Dec 2025 03:11:19 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: f8bb7b5cd838ab636347dc54fd0ab08ab3715d31 |
There was a problem hiding this comment.
Pull request overview
This PR adds interactive learner view functionality for activity pages, enabling learners to complete activities with answer checking and feedback, while providing administrators with preview capabilities.
- Implements viewer components for three activity types: multiple-choice, table, and matching activities
- Adds answer validation with feedback modals for incorrect answers
- Introduces preview modal for administrators to view activities from learner perspective
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/types/CourseTypes.ts | Updates type guard functions to accept optional parameters for safer null checking |
| frontend/src/theme/theme.ts | Fixes Pressed state color from black to proper theme color |
| frontend/src/components/pages/ViewModulePage.tsx | Integrates viewer components with role-based rendering and adds interactive buttons for learners and admins |
| frontend/src/components/course_viewing/table/TableViewer.tsx | New component implementing interactive table activity with answer checking |
| frontend/src/components/course_viewing/multiple-choice/MultipleChoiceViewer.tsx | Refactored to forwardRef pattern with answer validation logic |
| frontend/src/components/course_viewing/multiple-choice/MultipleChoiceViewOption.tsx | Adds visual feedback for correct answers and selected states |
| frontend/src/components/course_viewing/modals/WrongAnswerModal.tsx | New modal displaying hints when learners answer incorrectly |
| frontend/src/components/course_viewing/matching/MatchingViewer.tsx | New component implementing drag-and-connect matching activity with visual feedback |
| frontend/src/components/course_viewing/matching/MatchingBox.tsx | New component rendering individual matching boxes with selection states |
| frontend/src/components/course_authoring/matching/MatchingRow.tsx | Removes redundant label text from arrow component |
| frontend/src/components/course_authoring/editorComponents/PreviewLearnerModal.tsx | New modal allowing administrators to preview activities as learners |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export type ActivityViewerHandle = { | ||
| checkAnswer: () => void; | ||
| onRetry?: () => void; | ||
| }; |
There was a problem hiding this comment.
The ActivityViewerHandle type is duplicated here. It's also defined in TableViewer.tsx and MatchingViewer.tsx. This type should be defined once in a shared location and imported where needed to maintain consistency.
| <TableCell | ||
| align="center" | ||
| key={colIndex} | ||
| sx={{ backgroundColor: displayCorrect ? "#F5FFDF" : "transparent" }} |
There was a problem hiding this comment.
The magic color value "#F5FFDF" (light green) is duplicated across multiple files (TableViewer.tsx line 94, MultipleChoiceViewOption.tsx line 42, and MatchingBox.tsx line 30). This should be defined as a constant in the theme or a shared constants file to ensure consistency and make it easier to update.
|
|
||
| let backgroundColor: string | null = null; | ||
| if (displayCorrect) { | ||
| backgroundColor = "#F5FFDF"; // light green |
There was a problem hiding this comment.
The magic color value "#F5FFDF" (light green) is hardcoded here. This same value appears in other files (TableViewer.tsx, MatchingBox.tsx). It should be defined as a constant in the theme or a shared constants file to ensure consistency.
| import { TableActivity } from "../../../types/CourseTypes"; | ||
|
|
||
| type TableViewerProps = { | ||
| activity: TableActivity; | ||
| onWrongAnswer: () => void; | ||
| }; | ||
|
|
||
| export type ActivityViewerHandle = { | ||
| checkAnswer: () => void; | ||
| }; | ||
|
|
There was a problem hiding this comment.
The ActivityViewerHandle type is duplicated across multiple files (TableViewer.tsx, MultipleChoiceViewer.tsx, and MatchingViewer.tsx). This type should be defined once in a shared location (e.g., in CourseTypes.ts or a dedicated types file) and imported where needed to avoid duplication and potential inconsistencies.
| import { TableActivity } from "../../../types/CourseTypes"; | |
| type TableViewerProps = { | |
| activity: TableActivity; | |
| onWrongAnswer: () => void; | |
| }; | |
| export type ActivityViewerHandle = { | |
| checkAnswer: () => void; | |
| }; | |
| import { ActivityViewerHandle, TableActivity } from "../../../types/CourseTypes"; | |
| type TableViewerProps = { | |
| activity: TableActivity; | |
| onWrongAnswer: () => void; | |
| }; |
|
|
||
| type MatchingViewerProps = { | ||
| activity: MatchingActivity; | ||
| onWrongAnswer: () => void; | ||
| }; | ||
|
|
||
| export type ActivityViewerHandle = { | ||
| checkAnswer: () => void; | ||
| onRetry?: () => void; | ||
| }; | ||
|
|
There was a problem hiding this comment.
The ActivityViewerHandle type is duplicated here. It's also defined in TableViewer.tsx and MultipleChoiceViewer.tsx. This type should be defined once in a shared location and imported where needed to maintain consistency.
| type MatchingViewerProps = { | |
| activity: MatchingActivity; | |
| onWrongAnswer: () => void; | |
| }; | |
| export type ActivityViewerHandle = { | |
| checkAnswer: () => void; | |
| onRetry?: () => void; | |
| }; | |
| import type { ActivityViewerHandle } from "../table/TableViewer"; | |
| type MatchingViewerProps = { | |
| activity: MatchingActivity; | |
| onWrongAnswer: () => void; | |
| }; |
| <Typography variant="bodySmall"> | ||
| <Typography variant="bodySmall">{rowLabel}</Typography> | ||
| </Typography> |
There was a problem hiding this comment.
There's a redundant Typography wrapper. The rowLabel is wrapped in a Typography variant="bodySmall" component which itself is inside another Typography variant="bodySmall" component. The outer wrapper should be removed.
| <Typography variant="bodySmall"> | |
| <Typography variant="bodySmall">{rowLabel}</Typography> | |
| </Typography> | |
| <Typography variant="bodySmall">{rowLabel}</Typography> |
| <Button | ||
| sx={{ | ||
| height: "48px", | ||
| paddingLeft: "16px", | ||
| paddingRight: "24px", | ||
| paddingY: "10px", | ||
| gap: "8px", | ||
| border: "1px solid", | ||
| borderColor: theme.palette.Error.Light.Default, | ||
| borderRadius: "4px", | ||
| backgroundColor: theme.palette.Error.Light.Default, | ||
| color: theme.palette.Error.Dark.Default, | ||
| }} | ||
| onClick={() => {}} | ||
| > | ||
| <DeleteOutline /> | ||
| <Typography variant="labelLarge">Delete Page</Typography> | ||
| </Button> |
There was a problem hiding this comment.
The Delete Page button has an onClick handler that does nothing (empty function). This creates a non-functional button in the UI. Either implement the delete functionality or remove the button until it's ready to be implemented.
| <Button | |
| sx={{ | |
| height: "48px", | |
| paddingLeft: "16px", | |
| paddingRight: "24px", | |
| paddingY: "10px", | |
| gap: "8px", | |
| border: "1px solid", | |
| borderColor: theme.palette.Error.Light.Default, | |
| borderRadius: "4px", | |
| backgroundColor: theme.palette.Error.Light.Default, | |
| color: theme.palette.Error.Dark.Default, | |
| }} | |
| onClick={() => {}} | |
| > | |
| <DeleteOutline /> | |
| <Typography variant="labelLarge">Delete Page</Typography> | |
| </Button> |
| border: `1px solid ${theme.palette.Neutral[400]}`, | ||
| borderRadius: "4px", | ||
| cursor: item ? "pointer" : "default", | ||
| // transition: "border-color 120ms ease, box-shadow 120ms ease", |
There was a problem hiding this comment.
The commented-out transition property suggests it was intentionally disabled. If this is temporary, a TODO comment should explain why. If it's permanent, the commented code should be removed to keep the codebase clean.
| // transition: "border-color 120ms ease, box-shadow 120ms ease", |
Notion ticket link
Ticket Name
Implementation description
Steps to test
What should reviewers focus on?
Checklist