Conversation
There was a problem hiding this comment.
Pull request overview
Adds a “Lesson Resources” button (linking to student_lesson_plan_html_url) to progress UI, and introduces an isOnLevelView flag to suppress that button when progress is rendered in the level/header MiniView.
Changes:
- Add Lesson Resources button rendering in
SummaryProgressRowand gate resources buttons on!isOnLevelView. - Thread
isOnLevelViewthroughProgressTable→ group/table/row components and set it inMiniView. - Add/extend unit tests for the new resources-button behavior.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/test/unit/templates/progress/SummaryProgressRowTest.js | Adds tests asserting resources button presence/absence. |
| apps/test/unit/templates/progress/ProgressLessonTest.js | Adds a test asserting resources button hidden when isOnLevelView is true. |
| apps/src/templates/progress/SummaryProgressTable.jsx | Accepts and forwards isOnLevelView to rows. |
| apps/src/templates/progress/SummaryProgressRow.jsx | Adds resources button to summary rows; introduces layout styles and isOnLevelView gating. |
| apps/src/templates/progress/ProgressTable.jsx | Accepts isOnLevelView and forwards it (currently only to summary table in one branch). |
| apps/src/templates/progress/ProgressLesson.jsx | Hides existing resources button when isOnLevelView is true. |
| apps/src/templates/progress/LessonGroup.jsx | Forwards isOnLevelView to the selected table type. |
| apps/src/templates/progress/DetailProgressTable.jsx | Accepts/forwards isOnLevelView to ProgressLesson. |
| apps/src/code-studio/components/progress/MiniView.jsx | Passes isOnLevelView={true} into ProgressTable. |
Comments suppressed due to low confidence (1)
apps/src/templates/progress/ProgressTable.jsx:63
isOnLevelViewis passed toSummaryProgressTablein this branch but not toDetailProgressTable, so when the table is toggled to detail view (and especially in MiniView)ProgressLessonwill still render the Lesson Resources button. PassisOnLevelView={isOnLevelView}through here as well so both summary and detail views consistently hide/show resources.
<SummaryProgressTable
groupedLesson={groupedLessons[0]}
minimal={minimal}
isOnLevelView={isOnLevelView}
/>
</div>
<div style={isSummaryView ? styles.hidden : {}}>
<DetailProgressTable groupedLesson={groupedLessons[0]} />
</div>
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| it('shows lesson resources button when lesson has student_lesson_plan_html_url', () => { | ||
| const lessonWithUrl = { | ||
| ...fakeLesson('Maze', 1, false, 3), | ||
| student_lesson_plan_html_url: 'https://example.com/lesson-plan', | ||
| }; | ||
| const wrapper = setUp({ | ||
| lesson: lessonWithUrl, | ||
| }); | ||
|
|
||
| const button = wrapper.find('Button'); | ||
| expect(button).toHaveLength(1); | ||
| expect(button.props().href).toEqual('https://example.com/lesson-plan'); | ||
| expect(button.props().className).toEqual('ui-test-lesson-resources'); | ||
| }); | ||
|
|
||
| it('does not show lesson resources button when lesson has no student_lesson_plan_html_url', () => { | ||
| const wrapper = setUp(); | ||
|
|
||
| expect(wrapper.find('Button')).toHaveLength(0); | ||
| }); | ||
|
|
||
| it('does not show lesson resources button when lesson has student_lesson_plan_html_url and isOnLevelView is true', () => { | ||
| const lessonWithUrl = { | ||
| ...fakeLesson('Maze', 1, false, 3), | ||
| student_lesson_plan_html_url: 'https://example.com/lesson-plan', | ||
| }; | ||
| const wrapper = setUp({ | ||
| lesson: lessonWithUrl, | ||
| isOnLevelView: true, | ||
| }); | ||
|
|
||
| const button = wrapper.find('Button'); | ||
| expect(button).toHaveLength(0); | ||
| }); |
There was a problem hiding this comment.
These new tests are under describe('when viewing as Participant'), but they don't set viewAs: ViewType.Participant (baseProps defaults to Instructor). If the intent is to validate participant-only behavior, explicitly set viewAs for these cases (and consider adding a complementary instructor assertion, consistent with ProgressLesson).
| it('does not show Lesson Resources button when viewing as a participant and student_lesson_plan_html_url is not null, and isOnLevelView is true', () => { | ||
| let myLesson = defaultProps.lesson; | ||
| myLesson.student_lesson_plan_html_url = 'test-url'; | ||
| const wrapper = shallow( | ||
| <ProgressLesson | ||
| {...defaultProps} | ||
| lesson={myLesson} | ||
| viewAs={ViewType.Participant} | ||
| isOnLevelView={true} | ||
| /> | ||
| ); | ||
| expect(wrapper.find('Button').length).toEqual(0); | ||
| }); |
There was a problem hiding this comment.
This test mutates defaultProps.lesson in-place (let myLesson = defaultProps.lesson; myLesson.student_lesson_plan_html_url = ...) and doesn't clean it up. That can leak state across tests, especially when an assertion fails mid-test. Prefer cloning ({...defaultProps.lesson, student_lesson_plan_html_url: 'test-url'}) or deleting the property in a finally/afterEach.
| {lesson.student_lesson_plan_html_url && !isOnLevelView && ( | ||
| <Button | ||
| __useDeprecatedTag | ||
| className="ui-test-lesson-resources" | ||
| href={lesson.student_lesson_plan_html_url} | ||
| text={i18n.lessonResources()} | ||
| icon="file-text" | ||
| color="white" | ||
| target="_blank" | ||
| style={styles.buttonStyle} | ||
| /> | ||
| )} |
There was a problem hiding this comment.
The Lesson Resources button here is gated only on student_lesson_plan_html_url and !isOnLevelView, so it will also appear for instructors. ProgressLesson gates the same button on viewAs === ViewType.Participant; consider adding the same viewAs check here to keep behavior consistent between summary and detail progress views.
| {lesson.student_lesson_plan_html_url && !isOnLevelView && ( | |
| <Button | |
| __useDeprecatedTag | |
| className="ui-test-lesson-resources" | |
| href={lesson.student_lesson_plan_html_url} | |
| text={i18n.lessonResources()} | |
| icon="file-text" | |
| color="white" | |
| target="_blank" | |
| style={styles.buttonStyle} | |
| /> | |
| )} | |
| {lesson.student_lesson_plan_html_url && | |
| !isOnLevelView && | |
| viewAs === ViewType.Participant && ( | |
| <Button | |
| __useDeprecatedTag | |
| className="ui-test-lesson-resources" | |
| href={lesson.student_lesson_plan_html_url} | |
| text={i18n.lessonResources()} | |
| icon="file-text" | |
| color="white" | |
| target="_blank" | |
| style={styles.buttonStyle} | |
| /> | |
| )} |
Links
Testing story
Deployment notes
Privacy and security