-
Notifications
You must be signed in to change notification settings - Fork 253
Settings: Edit scan profile info #5793
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
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.
Thank you for the reviews @Vinnl and @codemist!
@Vinnl I’ve addressed the blocking issues as well as most of your comments. The only thing I could spend more time on is resolving the use of act()
in the unit tests, but I’d like to move forward with testing and not block the feature on those.
@codemist Happy to pair or elaborate more on anything if you have any open questions. The relevant backend for this has already landed with #5697.
...er_react)/(redesign)/(authenticated)/user/(dashboard)/settings/EmailAddressAdderRedesign.tsx
Show resolved
Hide resolved
src/app/(proper_react)/(redesign)/(authenticated)/user/(dashboard)/settings/actions.ts
Show resolved
Hide resolved
...er_react)/(redesign)/(authenticated)/user/(dashboard)/settings/EmailAddressAdderRedesign.tsx
Show resolved
Hide resolved
...er_react)/(redesign)/(authenticated)/user/(dashboard)/settings/EmailAddressAdderRedesign.tsx
Outdated
Show resolved
Hide resolved
...redesign)/(authenticated)/user/(dashboard)/settings/panels/SettingsPanelEditInfoRedesign.tsx
Outdated
Show resolved
Hide resolved
const l10n = useL10n(); | ||
const [profileFormData, setProfileFormData] = useState(props.profileData); | ||
const [ | ||
_updateProfileActionError, |
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.
That’s frustrating! After bf3825f the data should not get cleared.
const addEmailButton = screen.getByRole("button", { | ||
name: "Add email address", | ||
}); | ||
await act(async () => { |
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 issue is not the button itself, but the dialog that is getting shown. There are some side effects caused from it and also the input field in there. I’ll follow-up with those, but will move forward to not block the feature on these.
const addEmailButton = screen.getByRole("button", { | ||
name: "Add email address", | ||
}); | ||
await act(async () => { |
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.
See #5793 (comment).
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.
+1 for the change from deadname
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.
Approving, up to you if you want to merge this in or wait for Vincent's greenlight since many of the changes were addressing his feedback. Incredible work!
Cleanup completed - database 'blurts-server-pr-5793' destroyed, cloud run service 'blurts-server-pr-5793' destroyed |
@@ -21,6 +21,8 @@ import { ExperimentData } from "../../../../telemetry/generated/nimbus/experimen | |||
import MonitorLogo from "../../images/monitor-logo.svg"; | |||
import styles from "./Shell.module.scss"; | |||
import { UserAnnouncementWithDetails } from "../../../../db/tables/user_announcements"; | |||
import { ToastContainer } from "react-toastify"; |
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.
Would we need this in <ShellRedesign>
as well?
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.
Having it here is sufficient. The ToastContainer
sits outside the code path of the feature flag SidebarNavigationRedesign
, which toggles the redesigned shell.
References:
Jira: MNTOR-1806
Figma: https://www.figma.com/design/2iCgADpFXKOFZTEgCPkQsR/Settings?node-id=597-10052&t=c2UkGyKPdbvneuz5-4
Description
This PR adds a redesign for the
/user/settings/edit-info
view. US users with plus can see their profile scan info and also edit/add their details from/user/settings/edit-profile
. Any changes to the scan profile info should be reflected immediately and will be used with the next monthly scan. Things to note:Edit:
Screenshot
How to test
Locally
npm run db:migrate
SidebarNavigationRedesign
andEditScanProfileDetails
/user/settings/edit-info
Storybook
Checklist (Definition of Done)