-
Notifications
You must be signed in to change notification settings - Fork 479
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good call! i'll pull that into its own styles
key
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.
Great progress! LGTM!
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice job getting connected + unconnected components working!
</div> | ||
} | ||
</div> | ||
</div> |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies, @Erin007 , as I've sat quiet as these kind of extractions have been suggested in the past 😊
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 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:
MovingStudentsController
code