Vincent/notifications#407
Conversation
KudratAroraa
left a comment
There was a problem hiding this comment.
Requesting changes before merge.
The Notifications work itself looks correct and the intended feature for the web admin dashboard has been implemented properly. However, this PR currently includes additional changes from the app that are outside the scope of this task. Since this PR is specifically for adding notifications to the Guardian admin dashboard, I would recommend cleaning it up so that it only contains the relevant changes inside the guardian-admin-dashboard folder.
Please review the changed files again and keep only the dashboard-related notification changes in this PR. Any app-side changes that are unrelated to this specific task should be removed from this branch and raised separately if needed. Once the PR is scoped only to the intended admin dashboard work, it will be much easier to review and merge cleanly.
0d9a397 to
d7273af
Compare
d7273af to
68c8815
Compare
KudratAroraa
left a comment
There was a problem hiding this comment.
Approved (after accommodating the changes requested earlier)
I reviewed the PR and the notification functionality has been integrated into the admin dashboard layout in a meaningful way. The changes centralize notification state within AdminLayout, connect the notification flow into the Topbar, and add supporting interactions for viewing, deleting, and managing notifications through the drawer and modal-based flows. The overall structure remains aligned with the original layout while extending it with notification-related behaviour in a practical and organized way.
I also noted that the existing sidebar, mobile responsiveness, backdrop handling, and main outlet rendering flow remain preserved, which is important for avoiding regressions in the original dashboard layout. Passing the notification handlers and state down into the relevant components makes the overall implementation more cohesive and keeps the feature integrated at the layout level rather than scattering the logic across unrelated parts of the dashboard.
As a future improvement note, some of the notification-related UI styling and modal-related custom styles can later be moved into more dedicated or modular CSS files instead of continuing to expand shared global styling areas, so that the dashboard remains easier to maintain as more modules are added. Other than that, this looks good to merge.
Description
This PR integrates the notification system with the live backend API and ensures full data synchronization between the server and the frontend. Key changes include:
Reverting frontend data mapping in notificationService.js to consume the raw backend response.
Updating all notification-related components (NotificationPanel, NotificationDrawer, and AdminLayout) to use the MongoDB _id field instead of the previous mock id.
Ensuring status flags (isRead) and timestamps (createdAt) are correctly parsed from the backend schema.
Implementing robust error handling for API interactions (Fetch, Mark as Read, Delete).
Todos
How to test
[Add detailed steps to test the changes that have been made]
Screenshots and/or Gifs
Associated MS Planner Tasks
Known Issues
Notification Types: The current backend response does not include a type field (e.g., success, warning). As a result, all notifications currently default to the 'info' (primary color) styling.