[BD-05] [BB-2963] Improve Instructor dashboard #2
Conversation
There was a problem hiding this comment.
It's working well enough, I just have mostly minor concerns.
I'm just not sure of the iframe-based implementation. I assumed that would also be moved to React, but perhaps @giovannicimolin can answer that.
| @@ -11,7 +11,7 @@ function LoadingOrValue({ value }) { | |||
| } | |||
|
|
|||
| LoadingOrValue.propTypes = { | |||
| value: PropType.element, | |||
| value: PropType.oneOfType([PropType.element, PropType.number]), | |||
There was a problem hiding this comment.
I think having it be a node means it can be anything that can be rendered, so it will match, numbers, strings and other react elements.
| value: PropType.oneOfType([PropType.element, PropType.number]), | |
| value: PropType.node, |
src/ora-dashboard/OraDashboard.jsx
Outdated
| <DataTable data={data} /> | ||
| {status === RequestStatus.IN_PROGRESS && <Spinner animation="border" variant="primary" />} | ||
| {status === RequestStatus.SUCCESSFUL && ( | ||
| <div> |
There was a problem hiding this comment.
If this div isn't adding anything, you can remove it and just <> instead.
There was a problem hiding this comment.
Thanks for noticing that, yes a fragment would be better.
| * @param {string} key | ||
| * @param {string} direction | ||
| */ | ||
| const sort = function sort(firstElement, secondElement, key, direction) { |
There was a problem hiding this comment.
I think naming the function twice is a bit redundant.
| const sort = function sort(firstElement, secondElement, key, direction) { | |
| function sort(firstElement, secondElement, key, direction) { |
There was a problem hiding this comment.
Also this seems like a utility function, so can probably moved to a common places and imported as such.
| }, | ||
| ]; | ||
|
|
||
| console.log(sortableData.map(i => i.id)); |
There was a problem hiding this comment.
| console.log(sortableData.map(i => i.id)); |
src/ora-dashboard/OraDashboard.jsx
Outdated
|
|
||
| function OraDashboard({ data }) { | ||
| return ( | ||
| <main> | ||
| <div className="container-fluid"> | ||
| <h1>Open Responses</h1> |
| columns={columns.map(column => ({ | ||
| ...column, | ||
| onSort(direction) { | ||
| console.log('Sort in direction ', direction, column); |
There was a problem hiding this comment.
| console.log('Sort in direction ', direction, column); |
| }, [courseId, data]); | ||
|
|
||
| // create a copy of data for sortable Table | ||
| const sortableData = Object.values(data).slice().map(item => ({ ...item, actions: (<EmbedORAModal usageKey={item.id} title={item.name} buttonText="Manage" />) })); |
There was a problem hiding this comment.
I think the action here should only show up based on certain criterion, such as the availability of gradable items and staff access.
There was a problem hiding this comment.
Also I'm not sure "Manage" is a good enough label. Perhaps the column header can have a verbose anme, and this button can just be an icon.
Also make this translatable.
There was a problem hiding this comment.
Yeah, but to accommodate this condition we need staff_assessment flag available. Details on the ticket.
There was a problem hiding this comment.
@xitij2000, as we will be creating instructor API's to support this on MFE, I guess this Manage button will not be here anymore. It will be the same as the current instructor dashboard (clicking on numbers in Staff column). I will do that change after the APIs are created.
|
@giovannicimolin , does this PR have an associated Open edX JIRA ticket? |
This PR introduces a couple of improvements for the instructor dashboard in MFE -
edx/paragoncomponents to display data and loading spinnerJIRA tickets:
Discussions:Dependencies: None
Screenshots:
Embed Staff Grading -
Sandbox URL:Merge deadline: None
Testing instructions:
http://localhost:2003/<course_key>Author notes and concerns:
edx/paragonTablecomponent which is not stable yet. https://edx.github.io/paragon/components/tableX-frame-optionsdue to cross-site iframe embedding in production.Reviewers