EDUCATOR-4353 - ui - show override history #99
Conversation
This comment has been minimized.
This comment has been minimized.
Andytr1
force-pushed
the
andytr1/gradebook-show-override-history
branch
2 times, most recently
from
ef81580
to
53f5e90
Compare
Travis tests have failedHey @Andytr1, Node.js: lts/*npm run testTravisBuddy Request Identifier: 73edcf90-a24c-11e9-bee2-e9a995d9559a |
Andytr1
force-pushed
the
andytr1/gradebook-show-override-history
branch
from
53f5e90
to
e192e8f
Compare
| @@ -98,4 +98,8 @@ | |||
|
|
|||
| .mb-85 { | |||
| margin-bottom: 85px; | |||
| } | |||
|
|
|||
| .modal-dialog { | |||
There was a problem hiding this comment.
We might want to change the specificity of this. This will change the max-widths of all modals that we can use in our application which isn't desirable. Unfortunately, from the quick look I took, it seems as though the modal component gets placed at the top of the DOM so I'm not exactly sure its possible without editing the Modal component in paragon. @matthugs do you have any thoughts on this?
There was a problem hiding this comment.
Discussed offline using the parentSelector prop of paragon's modal component to achieve this (using portals!) https://edx.github.io/paragon/?path=/story/modal--basic-usage
src/components/Gradebook/index.jsx
Outdated
| @@ -433,13 +443,40 @@ export default class Gradebook extends React.Component { | |||
| open={this.state.modalOpen} | |||
| title="Edit Grades" | |||
| closeText="Cancel" | |||
| body={( | |||
| body={( // UI for this component | |||
There was a problem hiding this comment.
Please remove this comment. If you don't that it is clear that the jsx there is for the component, please turn it into a React functional component that way it can be referenced by a name: https://reactjs.org/docs/components-and-props.html
| data.override.possible_graded_override, | ||
| )); | ||
| }) | ||
| .catch(() => { |
There was a problem hiding this comment.
If the Linter lets you, remove this catch block entirely. If it doesnt at least remove the comments.
There was a problem hiding this comment.
I would probably add a StatusAlert that reports this issue to the educator, actually https://edx.github.io/paragon/?path=/story/statusalert--basic-usage
They're pretty quick to roll up; could show it in place of where the table would've shown
There was a problem hiding this comment.
For now, I used the existing pattern of dispatching errorFetching...
There was a problem hiding this comment.
But why did you do that? It seems as though this changes an errorFetching thing in our store but doesn't result in anything on the UI. Unless I'm missing something here. ...
| data.override.possible_graded_override, | ||
| )); | ||
| }) | ||
| .catch(() => { |
There was a problem hiding this comment.
I would probably add a StatusAlert that reports this issue to the educator, actually https://edx.github.io/paragon/?path=/story/statusalert--basic-usage
They're pretty quick to roll up; could show it in place of where the table would've shown
src/data/actions/grades.js
Outdated
| dispatch(gotGradeOverrideHistory( | ||
| formatGradeOverrideForDisplay(data.history), data.override.earned_all_override, | ||
| data.override.possible_all_override, data.override.earned_graded_override, | ||
| data.override.possible_graded_override, |
There was a problem hiding this comment.
I would pass along the subsectionId & userId we loaded this from as well, so that the reducer can keep track of which combinations have been loaded already, saving us from needing to hit the backend & make the user wait each time they switch which grade they're viewing.
Some pieces of the puzzle to doing that: a second argument is also provided to invoked thunks alongside dispatch; getState enables you to read from state to determine how you want your thunk to behave. E.g. if we wanted to read from a portion of state to determine whether we need to load data, we can get a reference to state by invoking that function. Simple example here: https://github.com/reduxjs/redux-thunk#motivation
There was a problem hiding this comment.
Future enhancement, probably
There was a problem hiding this comment.
I have a formal rule about this sort of code review discussion: if you are pushing off work for the future, show me tickets or I know you're lying ;).
If you could drop something in Jira linking back to this discussion then I'm more than happy to mark this as resolved.
There was a problem hiding this comment.
src/components/Gradebook/index.jsx
Outdated
| data={this.state.modalModel} | ||
| columns={[{ label: 'Date', key: 'date' }, { label: 'Grader', key: 'grader' }, | ||
| { label: 'Reason', key: 'reason' }, | ||
| { label: 'Adjusted grade', key: 'adjustedGrade' }]} |
There was a problem hiding this comment.
Looks like we should move this to a CONSTANT
There was a problem hiding this comment.
What would be the benefit of constant? I found an example in the file on line 284 creating a const userInformationHeadingLabel. Is this the same thing?
There was a problem hiding this comment.
To make intention clearer, and to make the code easier to read. It calls out an assumption, and pulls it up to a different point in code where it might be reused/moved to where it could be even more widely reused if necessary.
E.g. we could validate that the props provided to our component doesn't contain data that doesn't fit in these columns. Or we could calculate the column labels & the corresponding propTypes from the same, larger data structure, reducing duplicating this list in multiple places.
Andytr1
force-pushed
the
andytr1/gradebook-show-override-history
branch
from
e192e8f
to
32a692b
Compare
Andytr1
requested review from
reillz10 and
matthugs
and removed request for
MichaelRoytman
Andytr1
force-pushed
the
andytr1/gradebook-show-override-history
branch
from
32a692b
to
b37ab08
Compare
There was a problem hiding this comment.
I saw the errorFetchingGradeOverrideHistory event and I'm confused as to why thats there since it doesn't seem the errorFetching state isn't even mapped to props. That seems like a copy pasta of some code that should be cleaned up. That said, it sounds like @matthugs suggests that you put a statusAlert in there of some sort if there is an error. That can be up to you guys and product/ux to figure out the right behavior. For this PR, either fix that or remove this unused boilerplate for my thumbs up.
| } | ||
|
|
||
| .grade-history-assignment{ | ||
| padding-right: 49px; |
There was a problem hiding this comment.
NIT: These class names are super misleading and we would be better suited with more utility classes such as a pr-49 or something like that. That said, this is fine for now and can be refactored later.
| data.override.possible_graded_override, | ||
| )); | ||
| }) | ||
| .catch(() => { |
There was a problem hiding this comment.
But why did you do that? It seems as though this changes an errorFetching thing in our store but doesn't result in anything on the UI. Unless I'm missing something here. ...
updated unit tests EDUCATOR-4353 - major improvements to service etc remove unused unit tests fix code review continued update unit test after merge code review - showing error dialog
Andytr1
force-pushed
the
andytr1/gradebook-show-override-history
branch
from
b37ab08
to
2913f1d
Compare
| type="text" | ||
| name="reasonForChange" | ||
| value={this.state.reasonForChange} | ||
| onChange={value => this.onChange(value)} |
There was a problem hiding this comment.
One teachable moment point (I promise I'll space these out better in the future). If you define the function you're calling like this
onChange = (e) => {
this.setState({ [e.target.name]: e.target.value });
}then this can become
| onChange={value => this.onChange(value)} | |
| onChange={this.onChange} |
and you don't have to create a new function just to call another function with this set to the correct thing. This isn't particularly important, but understanding why this works can help you understand the difference between a function function and an =>/"arrow" function.
| updateUserId: userEntry.user_id, | ||
| }); | ||
| onChange(e) { | ||
| this.setState({ [e.target.name]: e.target.value }); |
There was a problem hiding this comment.
Nit: I would prefer a more explicit approach to writing these handlers, i.e. two separate & specifically named methods which handle only their specific use cases and can't be repurposed into something else.
I generally prefer to be very concrete in this sort of context. This method doesn't encapsulate enough to be worth abstracting.
| }]} | ||
| />)} | ||
|
|
||
| <div>Showing most recent actions(max 5). To see more, please contact |
There was a problem hiding this comment.
| <div>Showing most recent actions(max 5). To see more, please contact | |
| <div>Showing most recent actions (max 5). To see more, please contact |
src/data/actions/grades.js
Outdated
| dispatch(gotGradeOverrideHistory( | ||
| formatGradeOverrideForDisplay(data.history), data.override.earned_all_override, | ||
| data.override.possible_all_override, data.override.earned_graded_override, | ||
| data.override.possible_graded_override, |
There was a problem hiding this comment.
I have a formal rule about this sort of code review discussion: if you are pushing off work for the future, show me tickets or I know you're lying ;).
If you could drop something in Jira linking back to this discussion then I'm more than happy to mark this as resolved.
…dary Render ErrorPage component via app-level error boundary.
No description provided.