-
Notifications
You must be signed in to change notification settings - Fork 0
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
Pc reports page #62
Pc reports page #62
Conversation
This is to support the need for an admin to look at a specific report in detail on its own page.
needed exact match, not substring match
needed check for class of button
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider removing these commented out lines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im approving but i just put some minor style considerations
@@ -9,6 +10,18 @@ export const parameters = { | |||
actions: { argTypesRegex: "^on[A-Z].*" }, | |||
} | |||
|
|||
const currentUrl = window.location.href; | |||
const isLocalhost = currentUrl.startsWith("http://localhost:6006/"); | |||
const mockServiceWorkerUrl = isLocalhost ? "mockServiceWorker.js" : "https://" + window.location.hostname + "/mockServiceWorker.js"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im skeptical whether it is best practice to put ternary in a const var
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed, and decided to leave this as it is.
|
||
self.addEventListener('fetch', function (event) { | ||
const { request } = event | ||
const accept = request.headers.get('accept') || '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider using booleans instead of strings for readability
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is "do not touch" code from msw framework.
@@ -7,8 +7,11 @@ import LeaderboardPage from "main/pages/LeaderboardPage"; | |||
import AdminUsersPage from "main/pages/AdminUsersPage"; | |||
import AdminJobsPage from "main/pages/AdminJobsPage"; | |||
import AdminCreateCommonsPage from "main/pages/AdminCreateCommonsPage"; | |||
import AdminViewReportPage from "main/pages/AdminViewReportPage"; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extra line for a reason?
Overview
In this PR, we create a reports page where the Admin can view reports created by the InstructorReport job.
Merge these PRs first
Issues Addressed
Closes #18
Link to Storybook
TODO link to storybook
Details
This PR finishes work on the first draft of creating instructor reports.
/admin/reports
/admin/reports
/admin/reports/:report_id
; that page uses theReportHeaderTable
andReportDetailTable
to show the results.Test Plan (for interactive testing)