-
Notifications
You must be signed in to change notification settings - Fork 4
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
[DT-744] Add option to modify email preferences #2717
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.
This looks great, thank you! I do see that there are no unit tests for this page, it might be nice to add one with a minimal amount of coverage so we can start fleshing it out in future work. Alternatively, we can ticket fully testing it which is not a small amount of work.
LGTM - thx! |
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 code look OK, one question about the UX.
Send me email notifications | ||
</p> | ||
<FormField | ||
type={FormFieldTypes.YESNORADIOGROUP} |
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 the choices are Yes/No, does this need to use a radio button? In a case like this, I think a checkbox is equally clear and takes up less screen space
6e087f7
to
8583803
Compare
9fe7f3c
to
c4e6c38
Compare
Addresses
https://broadworkbench.atlassian.net/browse/DT-744
Summary
Add option to modify email preferences:
Screen.Recording.2024-11-07.at.9.33.49.AM.mov
Have you read Terra's Contributing Guide lately? If not, do that first.