-
Notifications
You must be signed in to change notification settings - Fork 1
MPDX-7076 Preferences #853
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
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
canac
left a 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.
Great work! There's a few things that can be cleaned up. I stopped commenting for every console.log, so search the project to find the rest. My comment about generated test-ids applies to all components as well.
I have two commits in this branch: one that improves the type safety of working users and people and another that makes the collapsible email and phone number styles better. I didn't want to commit directly to your branch, but you can cherry pick them into your branch.
pages/accountLists/[accountListId]/settings/preferences.page.tsx
Outdated
Show resolved
Hide resolved
pages/accountLists/[accountListId]/settings/preferences.page.tsx
Outdated
Show resolved
Hide resolved
pages/accountLists/[accountListId]/settings/manageAccounts.page.tsx
Outdated
Show resolved
Hide resolved
pages/accountLists/[accountListId]/settings/preferences.page.tsx
Outdated
Show resolved
Hide resolved
src/components/Settings/preferences/accordions/PrimaryOrgAccordion/PrimaryOrgAccordion.tsx
Outdated
Show resolved
Hide resolved
src/components/Settings/preferences/accordions/LocaleAccordion/LocaleAccordion.tsx
Outdated
Show resolved
Hide resolved
fdedfe8 to
9e8e73e
Compare
src/components/Settings/preferences/accordions/MpdInfoAccordion/MpdInfoAccordion.tsx
Outdated
Show resolved
Hide resolved
...ntacts/ContactDetails/ContactDetailsTab/People/Items/PersonModal/PersonEmail/PersonEmail.tsx
Outdated
Show resolved
Hide resolved
...ts/ContactDetails/ContactDetailsTab/People/Items/PersonModal/PersonEmail/PersonEmailItem.tsx
Outdated
Show resolved
Hide resolved
src/components/Settings/integrations/Organization/Modals/OrganizationAddAccountModal.tsx
Show resolved
Hide resolved
src/components/Settings/preferences/accordions/HomeCountryAccordion/HomeCountryAccordion.tsx
Outdated
Show resolved
Hide resolved
src/components/Settings/preferences/accordions/MpdInfoAccordion/MpdInfoAccordion.test.tsx
Outdated
Show resolved
Hide resolved
src/components/Settings/preferences/accordions/MpdInfoAccordion/MpdInfoAccordion.tsx
Outdated
Show resolved
Hide resolved
src/components/Settings/preferences/accordions/TimeZoneAccordion/TimeZoneAccordion.tsx
Outdated
Show resolved
Hide resolved
src/components/Settings/preferences/GetAccountPreferences.graphql
Outdated
Show resolved
Hide resolved
dr-bizz
left a 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.
I have left a few comments and recommendations.
We should create an index.page.ts at /settings which redirects to /settings/preferences
src/components/Layouts/Primary/TopBar/Items/ProfileMenu/ProfileMenu.test.tsx
Show resolved
Hide resolved
src/components/Settings/preferences/accordions/CurrencyAccordion/CurrencyAccordion.tsx
Outdated
Show resolved
Hide resolved
...preferences/accordions/HourToSendNotificationsAccordion/HourToSendNotificationsAccordion.tsx
Outdated
Show resolved
Hide resolved
src/components/Settings/preferences/accordions/TimeZoneAccordion/TimeZoneAccordion.tsx
Outdated
Show resolved
Hide resolved
src/components/Settings/preferences/accordions/TimeZoneAccordion/TimeZoneAccordion.tsx
Outdated
Show resolved
Hide resolved
...preferences/accordions/HourToSendNotificationsAccordion/HourToSendNotificationsAccordion.tsx
Outdated
Show resolved
Hide resolved
...preferences/accordions/HourToSendNotificationsAccordion/HourToSendNotificationsAccordion.tsx
Outdated
Show resolved
Hide resolved
pages/accountLists/[accountListId]/settings/preferences.page.tsx
Outdated
Show resolved
Hide resolved
canac
left a 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.
The only issue I see is that MPDX Angular uses the lowercase locale en-us and MPDX React uses the uppercase locale en-US. It appears to be an API issue though because it is en-us in the REST constants list and en-US in the GraphQL constants list. I can ping Shelby and I'll include you, Caleb. We may want to hold off on merging until that gets resolved though.
dr-bizz
left a 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.
The code looks good
Can you merge this into staging, so Scott can preview this for QA?
Luxon puts a space between the time and AM in node 18.19.1 but uses a NARROW NO-BREAK SPACE (U+202F) in node 18.13.0. The inconsistency between dev and CI environments caused failing tests.
...ts/ContactDetails/ContactDetailsTab/People/Items/PersonModal/PersonEmail/PersonEmailItem.tsx
Show resolved
Hide resolved
canac
left a 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.
Looks good! Was there anything in particular that you added that you wanted feedback on?
Nope, I just wanted to confirm before pushing it to prod. Scott and Robin already did QA last week. Do you think I need to ask them to do QA again? It seems like most of the changes I made they wouldn't notice a difference. |
|
@caleballdrin I'm not sure what Daniel thinks, but I think I'm good with merging. He and I have both been through it with a fine tooth comb multiple times, and Scott and Robin have looked over it too. I feel pretty confident that it works well, and if we do find an issue later on, we can always fix it then. |
dr-bizz
left a 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.
🔥💯🔥💯 - Great job
Description
Adding the Preferences page with Personal and Account accordions.
https://jira.cru.org/secure/RapidBoard.jspa?rapidView=3&view=detail&selectedIssue=MPDX-7076#
Checklist: