walmat / nebula-old

Deployment download link will be hosted here:
http://nebula-deployment.herokuapp.com
3 stars 0 forks source link

Add Notification Manager #77

Open pr1sm opened 6 years ago

pr1sm commented 6 years ago

Is your feature request related to a problem? Please describe. There are many notifications we need to be able to show to users (errors, successes, etc.). Each of these should be presented in a consistent manner with slight visual differences based on the type of notification.

Describe the solution you'd like A manager that is able to support creating a notification with a few preset styles (error, success, warning, info). This will allow us to trigger notifications using redux actions and handle them with a middleware. The middleware will use the notification manager to display notifications. The notification manager will be in charge of rendering the given notifications and handling the dismissal of notifications (by timeout or by user interaction).

Additional context We'll have to figure out the specific UX around notifications, but something basic like a popup message will do for now. Other notification UX designs should be researched and chosen.

The nice part about using a notification manager will be the ability to change the UX without changing the triggering mechanism (redux actions -> redux middleware -> notification function calls).

pr1sm commented 6 years ago

@walmat Do you think this needs to be in the stable alpha build, or will it be a new feature in the beta?

walmat commented 6 years ago

I like this idea a lot! If it’s not too complicated, having it in alpha isn’t a bad idea!

Sent with GitHawk

pr1sm commented 6 years ago

👍 Do you have any desired UX around this? A popup is one way to display this. It might be nice to also have a button the user could press to see all the recent notifications in a list.

pr1sm commented 6 years ago

I'll add this to the stable alpha milestone, we can always punt it to beta if we're getting short on time.

walmat commented 6 years ago

👍 Do you have any desired UX around this? A popup is one way to display this. It might be nice to also have a button the user could press to see all the recent notifications in a list.

@pr1sm I think it depends on the notification. If it’s a notification after a user saves a profile, a pop up might be nice if it’s styled well.

But what about if the user tries to save a profile and there are errors? Does it make sense to display the errors in a pop up as well? Or is it even necessary I guess.

Sent with GitHawk

pr1sm commented 6 years ago

Maybe we can roll this into a larger issue of notifying the user when the app is "working" on something (waiting for an api request, doing something else that takes time).

I can't remember where we talked about this (either in an issue comment thread or on discord), but I do remember having a discussion about a loading action to tell the UI to render some type of loading animation. This would integrate well into our existing thunks; it would look something like this:

const thunkAction = (...params) =>
  dispatch => {
+   dispatch(showLoading); // display the loading animation before promise...
    return _thunkRequest(...params) // start the promise...
      .then(response => {
        dispatch(_thunkAction);
+       dispatch(showSuccess); // display the success popup...
      })
      .catch(error => {
        dispatch(handleError);
+       dispatch(showError); // display the error popup...
      });
  };

Each of those actions would be handled by a middleware that uses the notification manager to render the popup without triggering the reducer chain.

walmat commented 6 years ago

Maybe we can roll this into a larger issue of notifying the user when the app is "working" on something (waiting for an api request, doing something else that takes time). ...

@pr1sm okay this makes sense! I think our previous talk was a loading screen between authentication and application launch. Like a simple gif or something, but this would look really good between actions!

Sent with GitHawk

pr1sm commented 5 years ago

Removing this from stable alpha since it's not a must have -- will tackle this after a couple of iterations.