feat: preview assessment from assessments page #604
Conversation
|
Visit the preview URL for this PR (updated for commit dc9b93f): https://jump-math-staging--pr604-joyce-preview-2irnaz1r.web.app (expires Sun, 17 Dec 2023 17:28:52 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: c42d8d0d853b05885664a2dd73f8245f4333ae51 |
| assessmentId, | ||
| }: { | ||
| assessmentId?: string; | ||
| }) => "/admin/assessment/" + assessmentId + "/preview"; |
There was a problem hiding this comment.
I think it would be kind of nice to overload the route here... i.e /admin/assessments/ + assessmentId + /preview, but I can't figure out why that isn't working & doesn't seem like a big enough issue to be a blocker... let me know if you have thoughts (:
There was a problem hiding this comment.
Yeah routing is tricky for this page... Happy to pair at some point if that would help. Or I can look into what's happening later this week
There was a problem hiding this comment.
Yah, we can pair on Sunday? This PR isn't a blocker so I don't mind leaving it open
There was a problem hiding this comment.
Same here, we resolved this synchronously right? 😅
frontend/src/components/admin/assessment-status/EditStatusButtons/PreviewButton.tsx
Outdated
Show resolved
Hide resolved
| @@ -272,22 +271,6 @@ const AssessmentEditor = ({ state }: AssessmentEditorProps): ReactElement => { | |||
| </VStack> | |||
| </VStack> | |||
| </Route> | |||
| <Route | |||
There was a problem hiding this comment.
Wondering why this change to remove the routes here was necessary? I think this might be the cause of the routing woes from your other comment. In particular, my understanding is that the AssessmentEditor component "owns" the route /admin/assessments/, which makes it capture all sub-routes. We could potentially rename the "owner" component to something more generic?
There was a problem hiding this comment.
@jfdoming I can't recall exactly, but we resolved this discussion synchronously right?
There was a problem hiding this comment.
I know something wasn't working that did require a code change. Did you commit that change?
| assessmentId, | ||
| }: PreviewButtonProps): React.ReactElement => { | ||
| const history = useHistory(); | ||
| const { data } = useQuery<{ test: TestResponse }>(GET_TEST, { |
There was a problem hiding this comment.
Iirc the tests table already loads this data for each row. Is there any way you could reuse that instead? Otherwise each user will be hammering the backend twice for each test that exists in the database
There was a problem hiding this comment.
Hmmm, but does it? Because the table runs the GET_ALL_TESTS query, right? Which is all the test metadata without the specific questions.
Here, we call GET_TEST which queries for the test metadata + questions for one specific test. I think this is similar to the implementation of EditButton.
Let me know if I am misunderstanding your comment here!
There was a problem hiding this comment.
Right. I'm wondering if it would be better to just add the additional data to the GET_ALL_TESTS query? Since imho our concern here might not be bandwidth but compute scale, since we have a single-replica base tier backend
There was a problem hiding this comment.
I guess I'm also thinking that EditButton could use a different implementation
Notion ticket link
Preview Assessment button in popover menu
Implementation description