Skip to content
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

Merged
merged 5 commits into from
Nov 13, 2024

Conversation

fboulnois
Copy link
Contributor

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.

  • Label PR with a Jira ticket number and include a link to the ticket
  • Label PR with a security risk modifier [no, low, medium, high]
  • PR describes scope of changes
  • Get a minimum of one thumbs worth of review, preferably two if enough team members are available
  • Get PO sign-off for all non-trivial UI or workflow changes
  • Verify all tests go green
  • Test this change deployed correctly and works on dev environment after deployment

@fboulnois fboulnois requested a review from a team as a code owner November 7, 2024 14:58
@fboulnois fboulnois requested review from pshapiro4broad and cinyecai and removed request for a team November 7, 2024 14:58
@fboulnois fboulnois changed the title Add option to modify email preferences [DT-744] Add option to modify email preferences Nov 7, 2024
Copy link
Contributor

@rushtong rushtong left a 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.

@lstrano-broad
Copy link

LGTM - thx!

Copy link
Member

@pshapiro4broad pshapiro4broad left a 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.

src/pages/user_profile/UserProfile.jsx Outdated Show resolved Hide resolved
Send me email notifications
</p>
<FormField
type={FormFieldTypes.YESNORADIOGROUP}
Copy link
Member

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

@fboulnois fboulnois force-pushed the fb-dt-744-update-email-preferences branch from 6e087f7 to 8583803 Compare November 13, 2024 17:32
@fboulnois fboulnois force-pushed the fb-dt-744-update-email-preferences branch from 9fe7f3c to c4e6c38 Compare November 13, 2024 19:14
@fboulnois fboulnois merged commit b4b1f12 into develop Nov 13, 2024
9 checks passed
@fboulnois fboulnois deleted the fb-dt-744-update-email-preferences branch November 13, 2024 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants