-
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
CSF201 registration form #26798
CSF201 registration form #26798
Conversation
…sf201_registration
…sf201_registration_form
…s and improvements/fixes to the workshop dashboard and enrollment form.
…stead of the course name. [skip ui]
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.
One tiny comment but overall this looks good!
<ButtonList | ||
id="attended_csf_intro_workshop" | ||
key="attended_csf_intro_workshop" | ||
answers={Object.keys(ATTENDED_CSF_COURSES_OPTIONS).map( key => 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.
You don't need to do the map
here - Object.keys(ATTENDED_CSF_COURSES_OPTIONS)
returns the same thing
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.
Thanks! Fixed.
} | ||
const processedCourses = []; | ||
this.state.csf_courses_planned.forEach((g) => { | ||
if (g === `${OTHER} ${EXPLAIN}`) { |
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.
What is this magic ===
?
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.
The triple equals checks that both value and type are the same. Potentially overkill in this particular block but it's consistent with the surrounding code and is safer anyway.
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.
Our linter requires it in many scenarios, although I'm not sure exactly what the rule is
@@ -45,6 +46,16 @@ export default class WorkshopEnrollmentSchoolInfo extends React.Component { | |||
this.props.onDelete(pendingDeleteId); | |||
} | |||
|
|||
csfCourseExperience(csf_course_experience) { |
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'm curious about our naming convention for method, should it start with a verb, or we consider this as an attribute?
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 think it should start with a verb. I'll fix that.
Adding the new questions to the CSF201 workshop enrollment form and showing those answers in the workshop dashboard. This does not turn on the ability to create a CSF201 workshop.
Workshop dashboard:
Enrollment form: