-
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
Manage students tab: show data with new UI under experiment #19948
Changes from all commits
53de3ac
3daba74
3334223
760c12e
5afef92
e348c9d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ import teacherSections, { | |
import SyncOmniAuthSectionControl from '@cdo/apps/lib/ui/SyncOmniAuthSectionControl'; | ||
import LoginTypeParagraph from '@cdo/apps/templates/teacherDashboard/LoginTypeParagraph'; | ||
import SectionsSharingButton from '@cdo/apps/templates/teacherDashboard/SectionsSharingButton'; | ||
import ManageStudentsTable from '@cdo/apps/templates/manageStudents/ManageStudentsTable'; | ||
|
||
/** | ||
* On the manage students tab of an oauth section, use React to render a button | ||
|
@@ -69,6 +70,38 @@ export function renderLoginTypeAndSharingControls(sectionId) { | |
); | ||
} | ||
|
||
export function renderSectionTable(sectionId, loginType) { | ||
const dataUrl = `/v2/sections/${sectionId}/students`; | ||
const element = document.getElementById('student-table-react'); | ||
|
||
$.ajax({ | ||
method: 'GET', | ||
url: dataUrl, | ||
dataType: 'json' | ||
}).done(studentData => { | ||
studentData = studentData.map((student) => { | ||
return { | ||
id: student.id, | ||
name: student.name, | ||
username: student.username, | ||
age: student.age, | ||
gender: student.gender, | ||
secretWords: student.secret_words, | ||
secretPicturePath: student.secret_picture_path, | ||
sectionId: sectionId, | ||
loginType: loginType, | ||
}; | ||
}); | ||
ReactDOM.render( | ||
<ManageStudentsTable | ||
studentData={studentData} | ||
id={sectionId} | ||
loginType={loginType} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we are going to be embedding loginType onto each student, should we just have ManageStudentsTable extract it from studentData (i.e. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. then when we have zero students, we won't have information we need to display the table headers properly There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahhh. I wonder if the better option would be to have ManageStudentsTable own sticking loginType onto studentData? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about I only do a mapping once, and I just be really explicit about everything I need and what I want the names to be? See above |
||
/>, | ||
element); | ||
}); | ||
} | ||
|
||
export function unmountLoginTypeAndSharingControls() { | ||
ReactDOM.unmountComponentAtNode(loginTypeControlsMountPoint()); | ||
ReactDOM.unmountComponentAtNode(shareSettingMountPoint()); | ||
|
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.
It's a little strange that some of our tab render functions we have inlined (renderSectionProjects, renderSectionProgress), and this one we import. Seems like it would be good for us to be consistent. Not something that needs to be fixed in this PR, but something to keep in mind.
FWIW, I think your approach of having it be imported might be better.
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 was following how some react buttons get loaded onto the page already. I think it makes a little more sense than defining it in index.