Manage Students Table: add multiple students button #20935
Conversation
| <div> | ||
| {numberOfEditingRows > 1 && | ||
| <Button | ||
| onClick={()=>{console.log("save all");}} |
There was a problem hiding this comment.
This is the TODO referenced in the description.
|
This PR is getting to be large so saving "Save all" button for another one. So close to being able to ship! |
|
Looks like the success notification still says "you added one student". Will that be confusing for users? |
|
|
||
| // New student row is created after a list of students have been | ||
| // added to the table, but their information hasn't been saved | ||
| // to the server yet. |
There was a problem hiding this comment.
These comments are really helpful in clarifying what's happening!
| isEditing: true, | ||
| isAddRow: true, | ||
| }; | ||
| // Creates a new "new student" add row for teach name in the array. |
There was a problem hiding this comment.
tiny nit: "each" instead of "teach"
| expect(sortedList[1].id).to.equal(2); | ||
| expect(sortedList[2].id).to.equal(1); | ||
| expect(sortedList[3].id).to.equal(3); | ||
| }); |
There was a problem hiding this comment.
I'm confused - this test makes it seem like the new student being added will go immediately after the add new student row, effectively at the top of the table. When brent and brad are added in the PR description, they're at the bottom of the list. I don't think it matters either way, I just want to double check the test matches the behavior and clarify if I'm reading it incorrectly.
There was a problem hiding this comment.
Oh... this is before you hit save - so the rows that are actively being edited show up at the top of the table.
There was a problem hiding this comment.
Yep, this is before hitting save. We want to pin them there because it'd be weird to have them scattered throughout the table. Order is clarified in the comments of sortRows function.
|
"you added one student" is currently the case because I haven't implemented save all so they are saving one at a time. Once I add save all, it'll have a specific number. |
| }; | ||
|
|
||
| state = { | ||
| openDialog: false, |
There was a problem hiding this comment.
Super-picky nit: I'd name this something like isDialogOpen. openDialog sounds like an action to me. In fact, you have a public method openDialog just below.
| /> | ||
| <BaseDialog | ||
| useUpdatedStyles | ||
| uncloseable |
There was a problem hiding this comment.
This dialog has a "Cancel" button. Is there a good reason to make it uncloseable? This is worth diverging from spec to provide user-friendly behavior, unless there's a specific reason for it.
There was a problem hiding this comment.
Nope - I copied this from somewhere and forgot to remove it.
| <div> | ||
| {i18n.addStudentsMultipleInstructions()} | ||
| </div> | ||
| <textarea name="textarea" rows="15" cols="70" ref="studentsTextBox"> |
There was a problem hiding this comment.
May not necessary to fix in this PR, especially if there will be a styling pass later, but it'd be nice to size this textbox to match the dialog width instead of using rows/cols. Also not sure it should be resizable.
There was a problem hiding this comment.
I'll do a style pass all together 👍
| // Types of rows in studentData/editingData | ||
| // newStudent looks like a studentRow with isEditing true, but | ||
| // is updated like an add row, since the student has yet to be added. | ||
| export const ROW_TYPE = { |
There was a problem hiding this comment.
Another nit: We're not nearly as consistent about this as we should be, but this doesn't follow our naming conventions for enums/constants.
Our primary styleguide calls for EnumNamesLikeThis, CONSTANT_VALUES_LIKE_THIS. That would change the ROW_TYPE enum to:
export const RowType = {
ADD: 'addRow',
NEW_STUDENT: 'newStudentRow',
STUDENT: 'studentRow',
};
// Used as `RowType.NEW_STUDENT`Same with the ADD_STATUS enum, above.
New React Manage Students tab is behind
manageStudentsexperiment, so this change won't affect users.rowTypeinstead of isAddRow in thestudentDatarowTypestodo in next PR: save all functionality