-
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
More copy + TeacherCon registration form updates #20376
Conversation
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.
LGTM, with a couple small suggestions
); | ||
} else if (data['course'] === 'CS Discoveries') { | ||
requiredFields.push( | ||
'howManyHours', | ||
'howManyTerms' | ||
'howManyTerms', | ||
'gradingSystem' |
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.
If this is required in both (all) cases, it could just be a static required field in the ruby class.
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.
After chatting w @mehalshah, we're going to leave this where it is in case course is ever something else.
</FormGroup> | ||
} | ||
|
||
{this.props.course === "CS Discoveries" && | ||
<FormGroup> | ||
{this.radioButtonsFor("howManyHours")} | ||
{this.radioButtonsFor("howManyTerms")} | ||
{this.radioButtonsWithAdditionalTextFieldsFor("gradingSystem", { | ||
[TextFields.otherPleaseList]: "grading_system_other" | ||
})} |
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.
Not a big deal but this could be reworked slightly to reduce repeated code, making the <FormGroup>
and these gradingSystem
radio buttons common, then branching for CSD/CSP for the course-specific fields.
Actually, CSP was missing more questions. Almost matched to spec now. It looks like there still should be a question "How many course hours per week will your school offer CS Principles?" but idk how to add a new question so I'll create a new card for that. |
@mehalshah PTAL at my addition: 3efaca8 |
…r consistency (the key format is transformed back and forth so it has no actual effect)
If approved to merge, I'll merge into staging after #20363.
gradingSystem
should be a question for both CSD and CSP