Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: preview assessment from teacher dashboard #605

Open
wants to merge 5 commits into
base: joyce/preview
Choose a base branch
from

Conversation

joyce-shi
Copy link
Contributor

@joyce-shi joyce-shi commented Oct 24, 2023

Notion ticket link

Preview Assessment screen in Teacher Dashboard

Implementation description

  • Preview assessment from TestSessionListItem
  • Preview assessment from Distribute Assessment page

@joyce-shi joyce-shi temporarily deployed to staging October 24, 2023 17:48 — with GitHub Actions Inactive
@github-actions
Copy link

github-actions bot commented Oct 24, 2023

Visit the preview URL for this PR (updated for commit fef2852):

https://jump-math-staging--pr605-joyce-teacher-previe-khzdlc7a.web.app

(expires Tue, 31 Oct 2023 18:38:42 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Sign: c42d8d0d853b05885664a2dd73f8245f4333ae51

@joyce-shi joyce-shi temporarily deployed to staging October 24, 2023 18:13 — with GitHub Actions Inactive
@joyce-shi joyce-shi temporarily deployed to staging October 24, 2023 18:27 — with GitHub Actions Inactive
@joyce-shi joyce-shi temporarily deployed to staging October 24, 2023 18:35 — with GitHub Actions Inactive
@joyce-shi joyce-shi marked this pull request as ready for review October 24, 2023 18:37
Copy link
Contributor

@jfdoming jfdoming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yayyyyy looks great! Had a couple of minor comments in the vein of preventing future bugs

}>(GET_TEST);

const previewAssessment = async (assessmentId: string) => {
const { data } = await previewAssessmentQuery({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: instead of checking for nullish data, we should catch exceptions here, since awaited Apollo lazy queries throw any error instead of returning it

}>(GET_TEST);

const onPreviewTest = async () => {
const { data } = await previewTest({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment!

<PopoverButton name="Delete" onClick={openDeleteModal} />
<>
<PopoverButton name="Delete" onClick={openDeleteModal} />
<Divider />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the extra divider here? I think VStack with the divider prop should automatically handle it, provided you remove the extra fragment (<>). (If we want to hide the whole popover for past sessions, it would make more sense IMO to either conditionally return null from this component, or (better) conditionally render the entire TestSessionListItemPopover component in the parent TestSessionListItem or wherever else this component is used.)

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

Successfully merging this pull request may close these issues.

2 participants