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

feat(notifications): add notification api and update banner #986

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

daniel-belcher
Copy link
Collaborator

Add parameter for notifications to be added as needed. Update Banner to use this system. Store dismissed locally.

🎫 Linked Ticket

ticket to close
related ticket

💬 Description / Notes

Initial dive into ticket was focused on database for dismissed notifications, as well as a more detailed table for notifications. This was deemed unnecessary and ticket was redone using a simple parameter for notification storage and to switch dismissed notifications to local storage so as not to have a table with User info.

🛠 Changes

  • New Lambda endpoint
  • Hook for Banner to pull notifications
  • Hook for dismissed notifications to be stored locally
  • No visual changes atm

Copy link
Contributor

github-actions bot commented Jan 8, 2025

Coverage Report

Status Category Percentage Covered / Total
🔵 Lines 81.36% 7188 / 8834
🔵 Statements 80.77% 7549 / 9346
🔵 Functions 75.08% 2155 / 2870
🔵 Branches 52.63% 1478 / 2808
File Coverage
File Stmts Branches Functions Lines Uncovered Lines
Changed Files
lib/config/deployment-config.test.ts 94.23% 81.25% 100% 94.11% 62, 71, 118
lib/config/deployment-config.ts 96.66% 93.33% 100% 96.66% 106-108
lib/lambda/getSystemNotifs.ts 0% 0% 0% 0% 4-23
react-app/src/api/useGetSystemNotifs.ts 33.33% 100% 0% 33.33% 6-8, 12-13
react-app/src/components/Banner/MMDLSpaBanner.tsx 9.09% 0% 0% 11.11% 8-19
react-app/src/hooks/useNotifications/index.ts 6.25% 0% 0% 6.66% 11-36
Generated in workflow #1482 for commit 9271f0f by the Vitest Coverage Report Action

Comment on lines +7 to +8
const { dismissed, clearNotif } = useNotifs();
const result = useGetSystemNotifs();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we consolidate these two hooks? Will they ever be used independently?

If yes, we can definitely move all this logic into the hook and let the component just concern itself with presentation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: READY PR is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants