Darien Classrooms & Assignments Mk2 - Part 2 #114
Conversation
PR UpdateI'm stopping development on this branch so it can be properly reviewed; of note, I'd seriously appreciate the reorganisation of components having a look-over. 👍 There's only one outstanding feature that will be spun off into its own PR:
That feature was a bit buggy because I was trying something smart (i.e. dumb) with the storage of Subject selection, so I'll resort to something simpler later. Also, Minor Weirdness:
StatusReady for review, with further features to be taken care of later. |
There was a problem hiding this comment.
Since this is largely duplication of the original components in the folders classrooms, this is mostly good to go. I'd like to have a better understanding of what the plan is for the Wildcam container components that hold the React lifecycle methods handling the API calls. Will the new Wildcam containers for the classrooms differentiate from the I2A needs? If so, then split makes sense. If not, then they should be shared somehow so we aren't duplicating code. I mostly wrote them with the intention that the API calls are shared. Should these move eventually into the classroom or assignments ducks to be shared there instead?
The views I understand will need to be different, especially the classroom editor since there aren't a fixed number of assignments. I'm not sure if @beckyrother has any time for input there, but if she does, it might be good to consult about it. I don't think a table will work if there's an unknown number of assignments.
The ClassroomForm I don't think needs its own Wildcam version? Unless you're planning on having fields specific to Wildcam?
You already know about the assignment creation weirdness on the WildcamClassroomManager, but I'll note what I'm seeing for subsequent PRs:
- The assignment list didn't update after the create form was submitted. I had to refresh to see the newly created assignment.
- Some CSS needs adjusting with the assignment delete button placement. It's not centered in its table cell.
- Should the edit button be after the assignment name? It just seems a bit odd to see the edit button for the classroom before the classroom name and the opposite with assignments
- The form doesn't load up the current assignment for editing if you click the edit button
| ? props.assignments[props.selectedClassroom.id] | ||
| : []; | ||
|
|
||
| const programURL = props.match.url.split('/'); // Remove once programs is in place with API |
There was a problem hiding this comment.
I see this is a comment I left for myself in the original ClassroomEditor. Non-blocker for this PR, but programs are in place now so this should be addressed. I think the slug property on programs would be used instead.
| <th id="student-name" scope="col"> | ||
| <span className="headers__header">Student Name/Zooniverse ID</span> | ||
| </th> | ||
| <th id="assignment-galaxy" scope="col"> |
There was a problem hiding this comment.
Wildcam would have different headers, so this id needs to be different.
| <th id="assignment-galaxy" scope="col"> | ||
| ABC | ||
| </th> | ||
| <th id="assignment-hubble" scope="col" > |
There was a problem hiding this comment.
Same comment about header id.
| ? <span>{student.zooniverseDisplayName}</span> | ||
| : <span className="secondary">{student.zooniverseLogin}</span> } | ||
| </td> | ||
| <td headers="assignment-galaxy">ABC</td> |
There was a problem hiding this comment.
Same comment about the header id
| : <span className="secondary">{student.zooniverseLogin}</span> } | ||
| </td> | ||
| <td headers="assignment-galaxy">ABC</td> | ||
| <td headers="assignment-hubble">ABC</td> |
There was a problem hiding this comment.
Same comment about the header id.
| removeStudentFromClassroom: () => {}, | ||
| //---------------- | ||
| showConfirmationDialog: false, | ||
| showCounts: { |
There was a problem hiding this comment.
This probably doesn't apply?
| removeStudentFromClassroom: PropTypes.func, | ||
| //---------------- | ||
| showConfirmationDialog: PropTypes.bool, | ||
| showCounts: PropTypes.shape({ |
There was a problem hiding this comment.
This probably doesn't apply for Wildcam.
|
|
||
| return Actions.createClassroom(classroomData) | ||
| .then((classroom) => { | ||
| if (!this.props.selectedProgram.custom) { |
There was a problem hiding this comment.
This only applies to I2A (a program with custom: false set). This is non-blocking comment and in my overall PR review, I'll go into the shared API request needs more.
PR Overview
This PR plans to improve the WildCam Darien Lab's Classrooms and Assignments systems, following up from Part 1 in #107 . I have an eye on making a generalised 'WildCam' Classroom & Assignment system (as opposed to being Darien-specific) but we'll see how that generalisation goes.
Goals:
Expected BIG STUFF that needs handling:
NOT in the scope:
Status
WIP