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.

Sign up for GitHub

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

Jump to bottom

Preferences notifications #821

Merged
merged 25 commits into from
Jan 11, 2024
Merged

Preferences notifications #821

merged 25 commits into from
Jan 11, 2024

Conversation

dr-bizz
Copy link
Contributor

@dr-bizz dr-bizz commented Nov 14, 2023

Description

Similar to #817, We've done a lot of work on Preferences. To prevent a 300-plus page PR I'm pushing certain changes into main which doesn't affect the user.

This PR is the notifications page in Preferences.

Changes

  • Adding Notification page with wrapper (Wrapper is the same as Connect Services PR.)
  • Adding Notification Skeleton
  • We do two graphQL queries, one to fetch the user's notifications preferences and the other to get the order the notifications should appear.

Checklist:

  • I have given my PR a title with the format "MPDX-(JIRA#) (summary sentence max 80 chars)"
  • I have applied the appropriate labels. (Add the label "On Staging" to get the branch automatically merged into staging.)
  • I have requested a review from another person on the project

@aws-amplify-us-east-1 AWS Amplify (us-east-1)
Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-821.d3dytjb8adxkk5.amplifyapp.com

@dr-bizz
Copy link
Contributor Author

dr-bizz commented Nov 14, 2023 •
edited
Loading

This PR is dependant on this PR https://github.com/CruGlobal/mpdx_api/pull/2677

canac
canac requested changes Nov 16, 2023
View reviewed changes
Copy link
Contributor

@canac canac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was only able to look at the code so far. I'll try running it locally later today.

pages/accountLists/[accountListId]/settings/notifications.page.tsx Outdated Show resolved Hide resolved
pages/accountLists/[accountListId]/settings/wrapper.tsx Outdated Show resolved Hide resolved
src/components/Settings/notifications/GetNotifications.graphql Outdated Show resolved Hide resolved
src/components/Settings/notifications/NotificationsTable.test.tsx Outdated Show resolved Hide resolved
src/components/Settings/notifications/NotificationsTable.test.tsx Outdated Show resolved Hide resolved
src/components/Settings/notifications/NotificationsTable.test.tsx Show resolved Hide resolved
src/components/Settings/notifications/NotificationsTable.test.tsx Show resolved Hide resolved
src/components/Settings/notifications/NotificationsTable.tsx Outdated Show resolved Hide resolved
src/components/Settings/notifications/UpdateNotifications.graphql Show resolved Hide resolved
@canac
Copy link
Contributor

canac commented Nov 16, 2023

It works great when I run it locally! The skeleton table while loading is a nice touch.

@dr-bizz dr-bizz requested a review from canac November 20, 2023 21:45
canac
canac requested changes Nov 21, 2023
View reviewed changes
Copy link
Contributor

@canac canac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple more tweaks and this should be good to go!

pages/accountLists/[accountListId]/settings/notifications.page.tsx Outdated Show resolved Hide resolved
src/components/Settings/notifications/NotificationsTable.test.tsx Outdated Show resolved Hide resolved
src/components/Settings/notifications/NotificationsTable.test.tsx Outdated Show resolved Hide resolved
src/components/Settings/notifications/NotificationsTableSkeleton.tsx Show resolved Hide resolved
src/components/Settings/notifications/NotificationsTable.tsx Outdated Show resolved Hide resolved
src/components/Settings/notifications/NotificationsTable.tsx Outdated Show resolved Hide resolved
src/components/Settings/notifications/NotificationsTable.tsx Outdated Show resolved Hide resolved
@dr-bizz dr-bizz requested a review from canac November 30, 2023 15:31
canac
canac approved these changes Dec 4, 2023
View reviewed changes
Copy link
Contributor

@canac canac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving, pending two minor tweaks.

src/components/Settings/notifications/NotificationsTable.tsx Outdated Show resolved Hide resolved
@dr-bizz dr-bizz added the On Staging Will be merged to the staging branch by Github Actions label Dec 15, 2023
@dr-bizz
Updating notifications to fix bug with main account viewing notificat…
54a118b 
…ions and removed fetching constants and now fetching notification types
@dr-bizz
tests for last commit
40f9e21
@dr-bizz dr-bizz merged commit 0485396 into main Jan 11, 2024
15 checks passed
@dr-bizz dr-bizz deleted the preferences-notifications branch January 11, 2024 21:27
canac added a commit that referenced this pull request Jan 12, 2024
@canac
Fix imports from #821
90cc6b6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@canac canac canac approved these changes

@caleballdrin caleballdrin Awaiting requested review from caleballdrin

Assignees
No one assigned
Labels
On Staging Will be merged to the staging branch by Github Actions
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dr-bizz @canac