Render ManageStudentTable on studio Teacher Dashboard #26562
Conversation
| export const summarizedSectionShape = PropTypes.shape({ | ||
| assignedTitle: PropTypes.string.isRequired, | ||
| code: PropTypes.string.isRequired, | ||
| course_id: PropTypes.number, |
There was a problem hiding this comment.
nit: can we convert all of these attributes from snake case (course_id) to camel case (courseId)? i'm not sure how heavy a lift that is, but i'd like all of this to be consistent on the javascript side
There was a problem hiding this comment.
Agreed - I like going for consistency. It's a bit of a lift since we share some of this information with other section shapes in studioHomepages. I'll address in a follow-up since it's a simple change, but one that could cause inadvertent problems if I miss something.
There was a problem hiding this comment.
yeah, that makes sense. that sounds good to me!
| // or any script in either course. | ||
| const scriptsToShowShareSetting = ["csp-2017", "csp-2018", "csd-2017", "csd-2018", "csd1-2018", "csd2-2018", "csd3-2018", "csd4-2018", "csd5-2018", "csd6-2018", "csd1-2017", "csd2-2017", "csd3-2017", "csd4-2017", "csd5-2017", "csd6-2017", "csp1-2018", "csp2-2018", "csp3-2018", "csp4-2018", "csp-explore-2018", "csp5-2018", "csp-create-2018", "csppostap-2018", "cspunit1", "cspunit2", "cspunit3", "cspunit4", "cspunit5", "cspunit6", "csp1-2017", "csp2-2017", "csp3-2017", "csp4-2017", "csp5-2017", "csp-explore-2017", "csp-create-2017", "csppostap-2017", "csp-post-survey"]; | ||
|
|
||
| if (scriptsToShowShareSetting.includes(section.script.name)) { |
There was a problem hiding this comment.
this long list of scripts are a bit of a red flag -- this would break after we added 2019 versions of these scripts, for example. Could we just say if (section.course) { instead?
| family_name = script.courses[0].properties["family_name"] | ||
| end | ||
| end | ||
|
|
There was a problem hiding this comment.
@davidsbailey What do you think about this approach? It solves for the problem of having to add script names every year. I need to make sure it works when:
- no script, no course assigned
- course is assigned
- script in a course is assigned
- script not in a course is assigned
I'm using script.courses; should I be concerned about times when a script is associated with more than one course? Does that happen or are scripts always specific to one course?
There was a problem hiding this comment.
nice solution @Erin007 ! Seems to be version-proof. One naming request though: section.script.family_name is misleading because scripts also have family names, e.g.
If you don't want to refactor this to be section.course.family_name, then you could do a name-only change to call this section.script.course_family_name. How does that sound?
There was a problem hiding this comment.
looking closer, I think section.course.family_name will be plenty easy, so that's what I'd recommend unless you see a reason not to.
|
@davidsbailey please take a look! you'll notice i ended up deleting the also, i left a TODO for myself to fix the duplication between |
|
Sorry for the long delay in responding, Maddie! I noticed that there is no dropdown to switch between sections in the new teacher dashboard. do we know if that is going to change? If we know that there will be a section switcher, then I'd think twice before removing the code to asynchronously load the section. But if this is not part of the current plan, then I'm happy to say YAGNI. Leaving in the duplication between teacherSectionsRedux and sectionDataRedux seems fine for now. |
|
thanks, @davidsbailey! i have some follow-up PRs to make for finishing the pegasus-to-dashboard teacher dashboard move, so i planned on adding the section dropdown in one of those. i'll double-check with poorva that we still want the dropdown, but i think that's still part of the plan |
Inching closer to removing Angular by rendering the React version of
ManageStudentsTablein the context of the new React teacher dashboard navigation at the teacher dashboard studio url.