Move students: Add section dropdown#22099
Conversation
| {id: 0, name: 'sectiona'}, | ||
| {id: 1, name: 'sectionb'}, | ||
| {id: 2, name: 'sectionc'} | ||
| ]; |
There was a problem hiding this comment.
This sections array is basically the same as the one used in the Story. If there are other similar constants used, we could consider extracting to their own file.
There was a problem hiding this comment.
I'd say you'll get an A+ for extracting a shared constant here, but I'll also add that we're generally much more lenient about allowing duplication in our stories and tests than we are in the rest of our codebase. in most tests and stories there would be no real danger for this data to change in one place but not the other, and I might only consider extracting test data if it becomes quite complex or if the format is changing a lot. For example we do have a generateFakeProjects method which is very useful because it produces fairly complex test data.
There was a problem hiding this comment.
Apologies, @Erin007 , as I've sat quiet as these kind of extractions have been suggested in the past 😊
| <input | ||
| required | ||
| name="sectionCode" | ||
| style={{...styles.input, width: 225}} |
There was a problem hiding this comment.
nit: you'll see it all over the place, but in general we try to avoid inline styles
There was a problem hiding this comment.
good call! i'll pull that into its own styles key
davidsbailey
left a comment
There was a problem hiding this comment.
Very nice, Maddie, this is great progress! I'm "approving with comments", meaning the suggestions I'm making are minor and I don't necessarily need to re-review after you address them unless something big ends up changing.
| }; | ||
|
|
||
| onChangeSection = (event) => { | ||
| const val = event.target.value; |
There was a problem hiding this comment.
nit: would be nice to give this a more descriptive name (moveType? moveTarget?), and also use it later in this function in place of event.target.value
| {id: 0, name: 'sectiona'}, | ||
| {id: 1, name: 'sectionb'}, | ||
| {id: 2, name: 'sectionc'} | ||
| ]; |
There was a problem hiding this comment.
I'd say you'll get an A+ for extracting a shared constant here, but I'll also add that we're generally much more lenient about allowing duplication in our stories and tests than we are in the rest of our codebase. in most tests and stories there would be no real danger for this data to change in one place but not the other, and I might only consider extracting test data if it becomes quite complex or if the format is changing a lot. For example we do have a generateFakeProjects method which is very useful because it produces fairly complex test data.
| export default MoveStudents; | ||
| export const UnconnectedMoveStudents = MoveStudents; | ||
|
|
||
| export default connect(state => ({ |
There was a problem hiding this comment.
nice job getting connected + unconnected components working!
| </div> | ||
| } | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
no need to do this right now, but you'll probably want to extract a subcomponent or two as this component grows.
There was a problem hiding this comment.
yeah, i was thinking about that. this component is definitely at its limit right now... i feel like there's a few places it could be broken up. maybe i'll pull the 'other teacher' functionality into its own component
There was a problem hiding this comment.
yeah, the "other teacher" stuff seems like a great candidate for extraction.
| {id: 0, name: 'sectiona'}, | ||
| {id: 1, name: 'sectionb'}, | ||
| {id: 2, name: 'sectionc'} | ||
| ]; |
There was a problem hiding this comment.
Apologies, @Erin007 , as I've sat quiet as these kind of extractions have been suggested in the past 😊
islemaster
left a comment
There was a problem hiding this comment.
Just wanted to say I trust Dave and Erin and you don't need to wait on my approval. 👍
This is the second in a series of PRs to rewrite the 'move students' functionality in React (first PR here).
Before
After
Righthand panel with a dropdown of the teacher's other sections plus an "other teacher" option:
If "other teacher" is chosen, additional fields appear for the teacher to input the other teacher's section code and choose whether to copy/move the students to the other teacher's section:
Work Remaining
This is obviously not finished, so I wanted to outline what is left to do for this feature:
MovingStudentsControllercode