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
Adding the AddParentEmailModal #33976
Conversation
</label> | ||
<div> | ||
<div style={styles.radioButton}> | ||
<label style={styles.input}> |
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.
why do we have labels without text, and why are the labels with the text not associated with their respective inputs?
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 labels without text are removed.
I'm not sure what you mean by the second bit.
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.
Labels can (edit: and should) be explicitly associated with the inputs they are labeling to support screen readers and provide some interfacing assistance (clicking the label focuses the input, etc).
You can either wrap the input in the label, like
<label>
<input />
{i18n.yes()}
</label>
or give the input an id
and the label a matching for
attribute, like
<input id="yes" />
<label for="yes">{i18n.yes()}</label>
See https://developer.mozilla.org/en-US/docs/Web/HTML/Element/label for more info
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! I used the htmlFor
attribute (React's version of for
) for the labels
We updated the text of the email opt-in this morning: do you mind changing it to "Can we email you with occasional updates on your child’s progress and projects, and updates about their course and computer science? (See our privacy policy)" |
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 but I'm no react expert!
First PR for the modal to add a parent email -- I'll follow up with a second one to add the controller. Screenshots below from Storybook so ignore the dark grey background.
spec
On load:
Invalid email:
Valid inputs:
Saving:
Links
Testing story
Reviewer Checklist: