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

Design a new notification system #463

Closed
landry314 opened this issue Sep 27, 2017 · 17 comments
Closed

Design a new notification system #463

landry314 opened this issue Sep 27, 2017 · 17 comments
Labels
[3] Feature Classification indicating the addition of novel functionality to the design [4a] Low Priority Priority indicating minimal impact to system/user -OR- an inexpensive workaround exists

Comments

@landry314
Copy link

landry314 commented Sep 27, 2017

Steps to Reproduce:

  1. Make sure you have more than one account "starred" so they are in the top pulldown
  2. Make sure you have very recent activity in the current active account
  3. Switch to another active account
  4. Switch back to the original active account

You should see a notification pop up for the last recent activity on the account you switch to.

  1. Switch back and forth again

You will keep seeing that notification pop up every time.

When a notification is dismissed it should never come back.

In the code there is something pulling up the last recent activity which simply should not be there.

I am not sure if the kind of activity or how old it is matters to reproduce but it does not always happen. For me, it was a "bought 14,842.77 BTS at 0.515 bitCNY/BTS". More testing might be needed.

@wmbutler
Copy link
Contributor

Agreed that once an announcement has been presented, it shouldn't be resent everytime a user switches back to the account.

@wmbutler wmbutler added this to the 171015 milestone Sep 28, 2017
@wmbutler wmbutler added the [3] Feature Classification indicating the addition of novel functionality to the design label Sep 28, 2017
@wmbutler wmbutler changed the title Switching Active Account Brings Up Last Recent Activity Notification [.5] Switching Active Account Brings Up Last Recent Activity Notification Sep 28, 2017
@landry314
Copy link
Author

This is hardly a feature request. This is a bug. It is not suppose to pull up the last recent notification informing you that a trade that happened 5 minutes ago just happened again... and again... and again... if you are switching active accounts a lot. It is disconcerting.

@wmbutler
Copy link
Contributor

wmbutler commented Oct 1, 2017

You have way too much time on your hands to quibble about categorization. It's not a bug if it's working as originally intended. And it is working as originally intended. I think the point of this is to discuss a change to the current behavior. "Annoying" things aren't bugs. They are just poor UI.

@landry314
Copy link
Author

Just trying to help, Bill.

@wmbutler wmbutler changed the title [.5] Switching Active Account Brings Up Last Recent Activity Notification [3] Design a new notification system Oct 11, 2017
@wmbutler
Copy link
Contributor

Notifications block various controls and are redundant when switching accounts. Design a new way to view notifications.

@wmbutler wmbutler modified the milestones: 171015, 171101 Oct 11, 2017
@wmbutler wmbutler self-assigned this Oct 11, 2017
@wmbutler wmbutler modified the milestones: 171101, 171115 Oct 15, 2017
@startailcoon
Copy link
Contributor

Can we replace the current notifications system with #686 perhaps?
I think any new notifications system should use browser/system notifications.

@wmbutler
Copy link
Contributor

wmbutler commented Nov 7, 2017

Some will choose not to enable browser notifications. Our notifications need to be self-contained in those cases.

That being said, notifications should be visible from anywhere in the application in a consistent location, but they should not obscure any part of the app during display. This presents a design problem because we are forced to reserve space for something that is temporal.

I've found that the least bad option is to reserve a very small horizontal band at the top of the application. Benefits:

  • Translates well to all sized devices
  • Is very noticeable to the end user
  • Takes up space, but the user tends to get used to the slightly larger gap at the top
  • Accommodates a lot of text for those larger messaages
  • Can incorporate different colors for say error - red, warning - yellow, info - green

Message Displayed

screen shot 2017-11-07 at 8 14 20 am

Message Not Displayed

screen shot 2017-11-07 at 8 21 59 am

@svk31
Copy link
Contributor

svk31 commented Nov 16, 2017

I don't like losing even more vertical space, and frankly I don't see why we should even replace the current notification system. The current one works on mobile and desktop, this new one will be bad for mobile especially but also for laptops with low vertical resolution.

The issue in the OP is a fairly simple one to fix, it's simply because we don't store any state keeping track of what the last operation notification was for each account. Adding that would fix the original issue..

@wmbutler
Copy link
Contributor

I don't like the current notification system because it:

  • Blocks areas of the screen while it's up
  • Stays on for a predefined amount of time disallowing other user actions
  • Has no color component to help the user understand whether it's informational or an error

@wmbutler wmbutler modified the milestones: 171119, 171201 Nov 17, 2017
@landry314
Copy link
Author

And it pulls up the last activity for that account when switching to an account, no matter how long ago it occurred... which is a bug.

@svk31
Copy link
Contributor

svk31 commented Nov 18, 2017

I don't like the current notification system because it:

  • Blocks areas of the screen while it's up

This can probably be worked around

  • Stays on for a predefined amount of time disallowing other user actions

There's a default timeout and a custom one, so you can set a specific timeout for different notifications

  • Has no color component to help the user understand whether it's informational or an error

This is not true, the current notifications have a color component.

This is the notification library that's currently being used: https://github.com/igorprado/react-notification-system

Building something to replace it will be timeconsuming and in my opinion completely unnecessary.

And it pulls up the last activity for that account when switching to an account, no matter how long ago it occurred... which is a bug.

I addressed this in my previous comment, it's simply a matter of adding localstorage state to track this, easy fix.

@wmbutler
Copy link
Contributor

If you can work around item 1, I'd be fine with that. My biggest issue with the notification right now is that it has to be dismissed before I can do anything else.

@wmbutler wmbutler removed their assignment Dec 2, 2017
@wmbutler wmbutler modified the milestones: 171205, 171215 Dec 3, 2017
@wmbutler wmbutler changed the title [3] Design a new notification system [5] Design a new notification system Dec 8, 2017
@svk31 svk31 added the [4a] Low Priority Priority indicating minimal impact to system/user -OR- an inexpensive workaround exists label Dec 13, 2017
@wmbutler wmbutler modified the milestones: 171215, 180105 Dec 14, 2017
@wmbutler wmbutler changed the title [5] Design a new notification system [6] Design a new notification system Dec 24, 2017
@wmbutler wmbutler modified the milestones: 180105, 180115 Jan 3, 2018
@wmbutler wmbutler modified the milestones: 180115, 180201 Jan 15, 2018
@wmbutler wmbutler modified the milestones: 180201, 180215 Feb 2, 2018
@gibbsfromncis
Copy link
Contributor

@wmbutler Lets do this big part step by step. I can prepare MVP of this feature and start to work if you agree.

@gibbsfromncis
Copy link
Contributor

First of all we should prepare the list of events we should notify user. We can start from our main operations:

For me MVP looks like this:

  • Add page where all notifications will be listed (on future we can add icon on header and dropdown but lets start without complexity)
  • User should be able to "mark notification as read"
  • Dismiss this kind of notifications on future.
  • Settings -> Notifications where user be able to enable/disable notifications types manually

It would be only Front-end Notification System so we can't notify users throw email about something. I'll track last operation ID on browser storage. And when user come back and fetch new operations I'll define new operations and add notifications about them.

On future we can add complex features like this:

  • Enable notifications only when user offline (for MVP it will works only on this way. But on future users be able to see notifications even he is online and on app)
  • Dismiss specific kind of notifications with specific params (like disable notifications for FILL ORDER of BTS/bitUSD only, or disable notifications of TRANSFER from specific user only)

@wmbutler
Copy link
Contributor

I'm closing this since we have a start for Chrome notifications now. Please re-open in a different issue if there are some continued features.

@wmbutler wmbutler changed the title [6] Design a new notification system Design a new notification system Feb 17, 2018
@wmbutler wmbutler removed this from the 180215 milestone Feb 17, 2018
@gibbsfromncis
Copy link
Contributor

gibbsfromncis commented Feb 17, 2018

@wmbutler Actually I've deployed Notifications for browsers. But for now we don't have any tab or place where we can see notifications which maybe interesting for users. In case of browser notifications user receive the message only if he have wallet opened. On another way he won't see any notification. Please read my (if you didn't) proposal - #463 (comment). I proposed you to create some place to collect all important events for users.

@wmbutler
Copy link
Contributor

I don't think we need notification history. We already have activity logs for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[3] Feature Classification indicating the addition of novel functionality to the design [4a] Low Priority Priority indicating minimal impact to system/user -OR- an inexpensive workaround exists
Projects
None yet
Development

No branches or pull requests

5 participants