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

[EASI-3918] Notification Page 🎉 #988

Merged
merged 61 commits into from
Mar 7, 2024
Merged

Conversation

garyjzhao
Copy link
Contributor

@garyjzhao garyjzhao commented Mar 5, 2024

EASI-3918

Changes and Description

  • Added new Notification routes
  • Implemented Info banner when there are no notifications
  • Notifications sorted in the order of most recent first
  • Notification read states
  • Link will update mutation to mark as read
  • User has option to "Mark all as read"
  • Show pagination after 10 items
  • Add Translation mappings
  • Add unit tests
  • Add cypress test

How to test this change

  1. Navigate to /notifications/
  2. Verify that info banner is there
  3. Create TWO discussions and tag self
  4. Verify that Notification on the menu bar has a red dot (notifying that there is an update)
  5. Go back to /notifications/
  6. Verify that new notifications are showing
  7. Hitting Mark all as read will remove the red dot and mark all them read
  8. Hitting View Discussion will only mark that individual discussion
  9. cypress and unit tests are not failing
  10. try to break it
  11. Test the Feature Flag in LD.

PR Author Review Checklist

  • Met the ticket's acceptance criteria, or will meet them in a subsequent PR.
  • Added or updated tests for backend resolvers or other functions as needed.
  • Added or updated client tests for new components, parent components, functions, or e2e tests as necessary.
  • Updated the Postman Collection if necessary.

PR Reviewer Guidelines

  • It's best to pull the branch locally and test it, rather than just looking at the code online!
  • Check that all code is adequately covered by tests - if it isn't feel free to suggest the addition of tests.
  • Always make comments, even if it's minor, or if you don't understand why something was done.

@garyjzhao garyjzhao marked this pull request as ready for review March 5, 2024 19:16
@garyjzhao garyjzhao requested review from a team as code owners March 5, 2024 19:16
@garyjzhao garyjzhao requested review from OddTomBrooks and patrickseguraoddball and removed request for a team March 5, 2024 19:16
Copy link
Contributor

@patrickseguraoddball patrickseguraoddball left a comment

Choose a reason for hiding this comment

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

Nice Gary! This all looks really good. I did notice some odd behavior when I tagged a user in a second model. The time elapsed is showing above/in between a notification. Here you can see 17 minutes is rendering between these two notifications. I think that could be caused by one of the notifications being a reply other than a tag. Since we are releasing this to Prod one at a time, we need to make sure side effects of other notifications are not appearing. Probably filtering the data for only the notifications we are currently supporting
Screenshot 2024-03-06 at 9 44 36 AM

const taggedNotifications = data?.currentUser.notifications.notifications.filter( notification => notification.activity.activityType === ActivityType.TAGGED_IN_DISCUSSION );

Other than that, a really great job

@patrickseguraoddball
Copy link
Contributor

patrickseguraoddball commented Mar 6, 2024

We will also need to reprocess the numUnreadNotifications, because that isn't accurate if we are only working with one type. We will need to calculate that manually for only the notification we have. This will need to be addressed in the menu bar icon, to only render the unread style icon if its of type TAGGED_IN_DISCUSSION

@patrickseguraoddball
Copy link
Contributor

I also noticed on mobile, the col="fill" makes the grid feel a bit crunched. Not a huge issue, but it looks a bit different than mobile grid in figma

Screenshot 2024-03-06 at 10 47 15 AM

@patrickseguraoddball
Copy link
Contributor

Also noticing some alignment/spacing issues. The gray boxes are supposed to the line up with the grid, and they seem to be extending past it. The icons should line up tight on the left grid as well

Screenshot 2024-03-06 at 11 58 35 AM

Copy link
Contributor

@patrickseguraoddball patrickseguraoddball left a comment

Choose a reason for hiding this comment

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

LG, nice Gary!

@garyjzhao garyjzhao merged commit 422af24 into main Mar 7, 2024
11 checks passed
@garyjzhao garyjzhao deleted the EASI-3918/notification-page branch March 7, 2024 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants