Shift3 / boilerplate-client-react

The Bitwise standard starting point for new React web projects.
7 stars 10 forks source link

Individual Notifications can now be marked as read #660

Closed joshwingreene closed 1 year ago

joshwingreene commented 2 years ago

Changes (Updated on Nov 30th)

  1. Adds the individual mark read networking request
  2. Made it so that clicking on either link of a notification will cause the mark_read endpoint to be called
  3. Renamed the markRead method in the UnreadNotification component to handleMarkAllRead
  4. Created a BaseNotification to standardize notification layout.
  5. Created the useMarkRead hook to handle the individual mark read functionality
  6. Adds a remove dispatch action to useLiveNotifications to remove individual notifications.
  7. Fixes the infinite loading functionality in the ReadNotifications component

Purpose

Individual Notifications can now be marked as read

Approach

  1. Besides passing the notification to the renderNotification method, the handleMarkRead method is passed as well. This is an optional parameter that was added to the renderNotification method, the AgentCreatedNotification component, and the newly added NotificationLink component. If handleMarkRead isn't undefined, then clicking on a link of an unread notification will cause the notification to be marked as read and cause the clear method to be called.

Testing Steps

  1. Pull in the changes to your local copy of this branch and run it alongside the dj_starter_demo repo.
  2. Create another admin. Use that admin to create one or more agents. Log back into the original admin and then open the notifications feature. Mark one or both notifications as being read and then check out the "Read" subtab.

Demo

https://user-images.githubusercontent.com/2876874/198729715-bd4f70ae-8978-4c6e-9c12-78c37747e1ce.mov

DropsOfSerenity commented 2 years ago

@joshwingreene Let's hold off on this one for now. I think we need a better solution here, this makes behavior too difficult to control between the various ways notifications are looked up, as notifications are also looked up when a new one comes in, and clicking on that notification should also mark it as read.

@joshwingreene Let's revisit this once I'm back after next week.

DropsOfSerenity commented 1 year ago

@joshwingreene I've reopened this and made a few changes, give it a look when you can :) Change summary is in the commit message.

joshwingreene commented 1 year ago

@DropsOfSerenity Yeah. Your changes make sense to me. Nice work!

I did notice one issue. Marking a notification as read via the toast message doesn't cause the list of read notifications to automatically update. Was this intentional?

DropsOfSerenity commented 1 year ago

Marking a notification as read via the toast message doesn't cause the list of read notifications to automatically update. Was this intentional?

Nope, we definitely wanna get this fixed before it goes in too.

joshwingreene commented 1 year ago

@DropsOfSerenity Understood. I'll go ahead and take care of this right now.

joshwingreene commented 1 year ago

@DropsOfSerenity I wasn't able to fix the issue that I brought up here and in yesterday's standup. However, I did correct an issue that was causing the infinite loading logic to not function properly. Previously, the notification list would be reset when the "Load More" button was clicked.