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

Add a third state to header icons #500

Closed
dcsjapan opened this issue Sep 16, 2015 · 6 comments
Closed

Add a third state to header icons #500

dcsjapan opened this issue Sep 16, 2015 · 6 comments
Assignees
Milestone

Comments

@dcsjapan
Copy link
Contributor

Currently the bell and flag icons have two states: orange with a number, blue with no number. This means that clicking the icon to reveal notifications (or reports) cancels the orange state and makes it look as if there are no unread notifications, even when there are.

How about adding a third state, as follows:

  • Orange with number: there are new unread notifications since you last checked here
  • Blue with number: there are unread notifications, but nothing new since you last checked
  • Blue with no number: there are no unread notifications

This would remind the user of unread notifications, while at the same time alerting him to new ones.

@franzliedke
Copy link
Contributor

I agree about this one. Will take it on. 👍

@franzliedke
Copy link
Contributor

I've made some progress on this. The idea is the following:

  • Calculate "unread" notifications in the NotificationList, by looping over the cached notifications, and filtering for unread notifications like this:
return app.cache.notifications.filter(notification => !notification.isRead()).length;
  • Show the number of "unread" notifications in the little bubble next to the dropdown button.
  • Only highlight (with color) the dropdown button when any notifications are "new".

Getting that last part ("new" notifications) right is the hardest part, I guess. Where should this state be kept, @tobscure? I've already taken care of the styling (only apply the accent color when a class "new" is applied to the button), but I'm hesitant about the data flow...

@tobyzerner
Copy link
Contributor

@franzliedke How about this approach:

  • Change User::getUnreadNotificationsCount() so that it doesn't care about time being more recent than notifications_read_time – i.e. it simply gets the number of notifications where not is_read.
  • Add a new method (User::getNewNotificationsCount()?) which does what getUnreadNotificationsCount used to do.
  • Output both of those values in the UserSerializer.
  • Always show the unreadNotificationsCount in the bubble on the dropdown button.
  • Only highlight the button if newNotificationsCount > 0.

@franzliedke
Copy link
Contributor

I implemented this, and it seems to work well. Even thought of edge cases. ;)

Can you please review, @tobscure? Especially the following things:

  • Does it make sense?
  • Did I use the correct LESS color variables?

That said, this still needs to be done for the flagged posts dropdown.

franzliedke added a commit to flarum/pusher that referenced this issue Sep 28, 2015
@kirkbushell
Copy link
Contributor

Do we maintain notifications beyond being read?

tobyzerner added a commit that referenced this issue Sep 29, 2015
Previously, clicking the "mark all notifications as read" button would individually mark each of the visible notifications as read. Since we now always show a badge with the number of unread notifications, we need to make sure that all notifications (not just the visible ones) can be marked as read. Otherwise it would be possible to get stuck with an unread badge there.

This commit adds a new API endpoint which marks *all* of a user's notifications as read. The JSON-API spec doesn't cover this kind of thing (updating all instances of a certain resource type), so I'm a bit unsure regarding what the endpoint should actually be. For now I've gone with POST /notifications/read, but I'm open to suggestions.

ref #500
@tobyzerner
Copy link
Contributor

@kirkbushell Yep we keep them around in case people want to refer back to them. But we should indeed think about having some kind of cleanup process that deletes notifications older than X days. Somewhat related to #217.

@franzliedke Great work! All looks good to me. See above commit for the only thing I added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants