Manage students tab: show data with new UI under experiment #19948
Conversation
caleybrock
changed the title
| @@ -9,6 +9,8 @@ import teacherSections, { | |||
| import SyncOmniAuthSectionControl from '@cdo/apps/lib/ui/SyncOmniAuthSectionControl'; | |||
There was a problem hiding this comment.
This file does not appear to be an entry point, so it probably should not be in sites/code.org/pages
There was a problem hiding this comment.
Where else would it go, just apps/src/code-studio/?
There was a problem hiding this comment.
Probably /templates/teacherDashboard/ (it's where I've been putting my stuff)
| @@ -69,6 +71,34 @@ export function renderLoginTypeAndSharingControls(sectionId) { | |||
| ); | |||
| } | |||
|
|
|||
| export function renderSectionTable(sectionId, loginType) { | |||
| if (experiments.MANAGE_STUDENTS) { | |||
There was a problem hiding this comment.
This will always be true. Need to check if it's enabled.
| dataType: 'json' | ||
| }).done(sectionData => { | ||
| console.log(sectionData); | ||
| sectionData = sectionData.map((section) => { |
There was a problem hiding this comment.
Might this more accurately be called studentData?
| <ManageStudentsTable | ||
| studentData={sectionData} | ||
| id={sectionId} | ||
| loginType={loginType} |
There was a problem hiding this comment.
Why do we both need to glom loginType onto sectionData AND provide it here?
There was a problem hiding this comment.
I remember being told that formatters for react-tabular should be const, meaning they don't have access to props, only rowData, but the table itself also needs the loginType because it toggles what columns and text to show.
If the formatters can be part of the class, which now thinking about it I don't see why not, we could just use the info from the top level component.
formatters now: https://github.com/code-dot-org/code-dot-org/blob/staging/apps/src/templates/manageStudents/ManageStudentsTable.jsx#L38
There was a problem hiding this comment.
If we were to leave it as is, we would also presumably want to add loginType to studentSectionDataPropType?
|
updated |
| url: dataUrl, | ||
| dataType: 'json' | ||
| }).done(studentData => { | ||
| studentData = studentData.map((section) => { |
There was a problem hiding this comment.
Each element in this array is a student, not a section, right? (Possible I'm misunderstanding)
There was a problem hiding this comment.
no you're right, I just missed that one apparently
| <ManageStudentsTable | ||
| studentData={studentData} | ||
| id={sectionId} | ||
| loginType={loginType} |
There was a problem hiding this comment.
If we are going to be embedding loginType onto each student, should we just have ManageStudentsTable extract it from studentData (i.e. studentData[0].loginType) rather than also passing it in separately?
There was a problem hiding this comment.
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.
Ahhh. I wonder if the better option would be to have ManageStudentsTable own sticking loginType onto studentData?
There was a problem hiding this comment.
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
| secretPicturePath: PropTypes.string, | ||
| secret_words: PropTypes.string, | ||
| secret_picture_name: PropTypes.string, | ||
| secret_picture_path: PropTypes.string, |
There was a problem hiding this comment.
whats the reason for changing these to snake_case? Why are some of these fields snake_case and some camelCase (i.e. sectionId/loginType)?
There was a problem hiding this comment.
That's what they're named in the API - I could also map the names before I pass them in.
There was a problem hiding this comment.
That's what I might do. I always find it confusing to have a mixture of different naming conventions in a single object.
| unmountLoginTypeAndSharingControls | ||
| } from './sections'; | ||
| unmountLoginTypeAndSharingControls, | ||
| renderSectionTable, |
There was a problem hiding this comment.
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.
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.
|
got an offline 👍 |
Still lots to do here - next step, redux.