Custom notifications in notification drawer #2001

benjaminapetersen · 2017-09-02T03:03:55Z

Integrates our internal notifications into the notification drawer.
Enhancement to #1326

spadgett · 2017-09-05T12:41:41Z

app/scripts/directives/notifications/notificationDrawerWrapper.js

+      // internal notifications have different types than API events
+      var notificationEventTypeMap = {
+        success: 'Normal',
+        error: 'Warning'


We should handle all four types in the drawer: info, success, error, and warning. Then we just map event "normal" -> "info"

Eliminating both maps via PR to common

spadgett · 2017-09-05T12:42:49Z

app/scripts/directives/notifications/notificationDrawerWrapper.js


+      var iconClassByEventSeverity = {
+        Normal: 'pficon pficon-info',
+        Warning: 'pficon pficon-warning-triangle-o'


We should check what toast notifications are doing for these icon mappings. Ideally we have one map somewhere so we're always consistent.

Looks like they use alertIcon :

<span class="{{notification.type | alertIcon}}" aria-hidden="true"></span>

I'll update & use that

spadgett · 2017-09-05T12:44:35Z

app/scripts/directives/notifications/notificationDrawerWrapper.js

-        notificationListener = null;
+      var makeProjectGroup = function(projectName, notifications) {
+        return {
+          heading: $filter('displayName')(projects[projectName]) || projectName,


The || is not necessary since displayName default to name if there's no display name.

var project = projects[projectName] return { heading: $filter('displayName')(project), project: project, notifications: notifications };

spadgett · 2017-09-05T12:47:54Z

app/scripts/directives/notifications/notificationDrawerWrapper.js

+                    };
+                  }
+                  return result;
+                }, {});


@benjaminapetersen I know you prefer this indentation style, but the rest of the code uses

return _.reduce(events, function(result, event) { // ... });

Let's try to keep consistent styles.

I'll update this.

Ref #1121 for when we have time.

spadgett · 2017-09-05T12:50:31Z

app/scripts/directives/notifications/notificationDrawerWrapper.js

+        var proj = $routeParams.project;
+        return _.orderBy(
+                  _.assign({}, firstMap[proj], secondMap[proj]),
+                  ['event.lastTimestamp', 'event.firstTimestamp'],


We need to be assigning timestamps to events in notification service to get proper ordering.

I can follow-up a PR to origin-web-common to do some work on NotificationsService. This line should just be a fallback.

Sure, OK as a follow on

spadgett · 2017-09-05T12:56:29Z

app/scripts/directives/notifications/notificationDrawerWrapper.js

-          drawer.drawerHidden = true;
+      var deregisterEventsWatch = function() {
+        if(eventsWatcher) {
+          DataService.unwatch(eventsWatcher);


eventsWatcher = null;

spadgett · 2017-09-05T12:57:25Z

app/scripts/directives/notifications/notificationDrawerWrapper.js

-      //   clientGeneratedNotifications.push(notification);
-      // };
+      var notificationWatchCallback = function(event, notification) {
+        var uid = notification.id || _.uniqueId('notification-');


Ideally we should add in an ID for those that don't have one in NotificationsService.

spadgett · 2017-09-05T13:49:21Z

app/scripts/directives/notifications/notificationDrawerWrapper.js

+          unread: true,
+          uid: uid,
+          // emulating an API event to simplify the template
+          event: {


I'd rather not add a dummy event. I'd rather copy necessary properties like uid and timestamp from the event to top-level properties, and use those in the view.

ok ill update

spadgett · 2017-09-05T13:49:51Z

app/scripts/directives/notifications/notificationDrawerWrapper.js

+          uid: uid,
+          // emulating an API event to simplify the template
+          event: {
+            lastTimestamp: notification.lastTimestamp || moment.parseZone(new Date()).utc().format(),


Better to add the timestamp in NotificationsService

spadgett · 2017-09-05T13:51:08Z

app/scripts/directives/notifications/notificationDrawerWrapper.js

+        if(!projectName) {
+          return;
+        }
+        notificationListener = $rootScope.$on('NotificationsService.onNotificationAdded', cb);


We should be able to add this once instead of registering and removing on project changes.

True. no need to update this one like the events watch.

benjaminapetersen · 2017-09-06T01:04:43Z

Updated. Will need a release of origin-web-common with PR 170 before this can go.

dtaylor113 · 2017-09-06T14:22:58Z

Hi @benjaminapetersen, The 'Clear All' doesn't appear to be working correctly. I start out with 3 notifications:

I think click on an 'X' to remove one:

Then click on 'Clear All' and all three re-appear instead of removing them all as I was expecting:

dtaylor113 · 2017-09-06T14:27:18Z

Hi, the 'View Build' link in this notification doesn't seem to do anything/navigation anywhere:

benjaminapetersen · 2017-09-06T14:37:42Z

Thanks @dtaylor113! I made a few updates last night & have another push coming this morn that addresses these items. I updated EventsService a bit & am reconciling the ids.

benjaminapetersen · 2017-09-06T14:53:56Z

Updated & squashed the last 5 commits down.
Think I need a rebase on latest master, thats coming in a few mins.

benjaminapetersen · 2017-09-06T15:07:45Z

btw, Id's are still janky w/o the update to origin-web-common. I'll PR a version bump.

benjaminapetersen · 2017-09-06T16:19:00Z

Rebased, also put a fallback id in otherwise the notifications just act too weird (until common version bump happens).

spadgett · 2017-09-06T17:13:10Z

app/scripts/services/events.js

-      var markRead = function(event) {
-        _.set(cachedEvents, [event.metadata.uid, READ], true);
+      var markRead = function(id) {
+        _.set(cachedEvents, [id, READ], true);
        BrowserStore.saveJSON('session','events', cachedEvents);


If we're saving read and clear status by ID in session storage, we should really change NotificationService to generated a UID instead of using _.uniqueId. With _.uniqueId the IDs will be reused if you refresh the page, and notification might be incorrectly hidden from the drawer.

This is true. I could pull the generateSecret function from ApplicationGenerator:

scope._generateSecret = function(){ //http://stackoverflow.com/questions/105034/create-guid-uuid-in-javascript function s4() { return Math.floor((1 + Math.random()) * 0x10000) .toString(16) .substring(1); } return s4()+s4()+s4()+s4(); };

Simpler could just be _.uniqueId('notification_') + Date.now().

spadgett · 2017-09-06T17:28:22Z

Looks like the sort is still wrong. (I should have latest common changes with the timestamps.)

benjaminapetersen · 2017-09-06T19:21:24Z

@openshift/team-ux-review

Need some feedback! We have our notifications integrated into the drawer with the curated api events. This creates some quirks that we think need to be addressed before we can merge this feature.

As a user, if I click deploy I will get 1. a toast indicating the action (deploy) and a notification indicating the same action. I know I initiated the action, so double affirmation is a bit much.
If I close the toast, I might expect that the notification in the drawer should be marked read. It is no longer news to me.
The API Events have only 2 types, Warning and Normal. Our notifications have 4: Warning, Error, Success, Info. This can cause mismatch, like with the icons here:

These two events should be the same level (actually the "Deployment Created" event might be more weighty than "Deployment Started"), but the icons seem to indicate that "Deployment Created" is of lesser value.

There is a difference in the way we format the two event types. I'll add another screenshot after Start Build to illustrate this further:

With the API event we link the name of the object, with the internal notification we add a view build link as a separate component.

I really think we should add the counts back to the header, it is definitely easy to miss a "warning" event that gets pushed below the fold:

However, since internal events have 4 states (explained above) we probably don't want 4 icons with counts in the header.

We have discussed that the large toast for a success message is perhaps more obtrusive than it needs to be:

It might be ideal to make these smaller similar to how gmail gives a centered sent message toast. This could solve the problem of toasts frequently covering action buttons:

And save the large toasts for bad things:

We have also discussed adding a way to configure what internal events end up as a toast, and which can just go directly into the drawer.

Thoughts?

Observations gathered from earlier discussion with @ncameronbritt @spadgett

ncameronbritt · 2017-09-06T19:55:54Z

The toast notifications are helpful when a user takes an action in the sense that they provide feedback to the user. But obviously toasts aren't the only way we could provide the user feedback that the system registered their input. The little message like gmail provides is another option. Having the button itself do something is yet another. May be worth thinking about this from a PF perspective too for actions that don't obviously change a user's context.

benjaminapetersen · 2017-09-06T20:14:28Z

Right! @ncameronbritt you suggested something like this for a button effect:

I dunno why github made the gif huge 😕

ncameronbritt · 2017-09-06T20:21:03Z

Right on @benjaminapetersen! I like that that communicates that something is happening right in the place where the user took an action.

serenamarie125 · 2017-09-06T21:43:21Z

Looks cool, but not consistent with what we do in PatternFly. I'd suggest we wait on that behavior and do some
Research to see if that's what we want to consistently throughout Openshift and the rest of the portfolio. Or in what situations we want to do it :)

spadgett · 2017-09-06T22:41:05Z

@serenamarie125 We've had some feedback from @smarterclayton and @sspeiche that some of the confirmation toasts are annoying. ("I know I just clicked that button. Why are you telling me?") See also #1911. I remember you and @jeff-phillips-18 had similar concerns.

The problem is that on some pages, depending on what tab is selected, nothing on the page changes in response to an action. So it can look like the button didn't do anything. One example is clicking "Start Build" on the build config page with the "Configuration" tab selected. Nothing changes, and it seems like the action didn't work. The user might try clicking the button several times and accidentally creating several builds.

What we'd really like is a way to let users know an action succeeded that is a little less in-your-face than a toast. A more subtle, "Yep, that worked" message. The Gmail screenshot from @benjaminapetersen is good example. The button changing is also interesting, although I'd like something that works in contexts where there isn't a button.

Do you think this make sense to explore as a pattern in Patternfly?

serenamarie125 · 2017-09-07T03:03:30Z

@spadgett I agree that the toasts can be annoying. IMO, we should be identifying a strategy for default usage of toasts & availability in the notification drawer. Ultimately the user should be able to configure what they would like to see where as well.

That being said, I do agree that we need a solution so that users are aware that they have performed an action. @spadgett is this something we can do for 3.8? If so, i think that will give us the ability to perform some research and come up with a solution which should be used for similar use cases across products. (thus my answer is yes i think we should explore a PatternFly solution 😄 )

serenamarie125 · 2017-09-07T03:04:58Z

@jeff-phillips-18 shouldn't the toast be UNDER the top banner? Maybe this is a bug in PatternFly, although I thought we had fixed it?

dtaylor113 · 2017-09-18T18:28:15Z

Hi, common was bumped to 0.0.57

benjaminapetersen · 2017-09-18T18:43:33Z

@dtaylor113 yup think this PR will wait on all the version bumps in #2103.

benjaminapetersen · 2017-09-19T15:21:44Z

Reviewed & rebased.

[test]

Quite a few moving parts here, once I finish the test overhaul I want to circle back around & provide some coverage to these components.

spadgett · 2017-09-19T15:36:47Z

[merge]

benjaminapetersen · 2017-09-19T18:25:40Z

########## FINISHED STAGE: FAILURE: PROVISION CLOUD RESOURCES [00h 02m 39s] 
##########

[merge]

benjaminapetersen · 2017-09-19T18:41:12Z

wake up...

[merge]

benjaminapetersen · 2017-09-19T19:05:06Z

Another flake

########## FINISHED STAGE: FAILURE: PROVISION CLOUD RESOURCES [00h 02m 39s] ##########
Build step 'Execute shell' marked build as failure
[PostBuildScript] - Execution post build scripts.

benjaminapetersen · 2017-09-19T19:31:10Z

flakes, and travis keeps hanging...

[merge]

spadgett · 2017-09-19T20:23:43Z

Needs rebase

- previously only a curated list of API Events made it into the drawer - this change catches all "internal events" currently experienced as toasts & brings them into the drawer - future improvement may be to suppress the "toast" for certain internal events so as not to excessively message the user

benjaminapetersen · 2017-09-19T20:51:27Z

rebased again.

openshift-bot · 2017-09-19T20:55:35Z

Evaluated for origin web console test up to 966b580

openshift-bot · 2017-09-19T21:15:34Z

Evaluated for origin web console merge up to 966b580

openshift-bot · 2017-09-19T21:15:43Z

Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin_web_console/232/) (Base Commit: c63e2f5) (PR Branch Commit: 966b580)

openshift-bot · 2017-09-19T21:16:27Z

Origin Web Console Test Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_web_console/236/) (Base Commit: 2041c75) (PR Branch Commit: 966b580)

benjaminapetersen force-pushed the bpeterse/notifications-to-notification-drawer branch from 1c9e134 to 697843e Compare September 2, 2017 03:04

spadgett requested changes Sep 5, 2017

View reviewed changes

This was referenced Sep 5, 2017

Update notifications with id and timestamp openshift/origin-web-common#170

Merged

Null out handle in DataService.unwatch openshift/origin-web-common#171

Closed

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 6, 2017

benjaminapetersen force-pushed the bpeterse/notifications-to-notification-drawer branch from 4d1a986 to 8316f01 Compare September 6, 2017 14:53

benjaminapetersen mentioned this pull request Sep 6, 2017

Bump version to 0.0.53 openshift/origin-web-common#172

Merged

benjaminapetersen force-pushed the bpeterse/notifications-to-notification-drawer branch from 8316f01 to 32aa7ff Compare September 6, 2017 16:18

spadgett reviewed Sep 6, 2017

View reviewed changes

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 6, 2017

benjaminapetersen force-pushed the bpeterse/notifications-to-notification-drawer branch from 32aa7ff to 327b0f7 Compare September 6, 2017 19:32

benjaminapetersen mentioned this pull request Sep 6, 2017

Improve notification id openshift/origin-web-common#174

Merged

benjaminapetersen force-pushed the bpeterse/notifications-to-notification-drawer branch from 327b0f7 to 86ebd5c Compare September 6, 2017 19:45

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 7, 2017

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 18, 2017

benjaminapetersen force-pushed the bpeterse/notifications-to-notification-drawer branch 3 times, most recently from 7cb97e9 to 7b7a4b5 Compare September 19, 2017 15:21

spadgett removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 19, 2017

spadgett approved these changes Sep 19, 2017

View reviewed changes

benjaminapetersen closed this Sep 19, 2017

benjaminapetersen reopened this Sep 19, 2017

benjaminapetersen closed this Sep 19, 2017

benjaminapetersen reopened this Sep 19, 2017

benjaminapetersen force-pushed the bpeterse/notifications-to-notification-drawer branch from 7b7a4b5 to 966b580 Compare September 19, 2017 20:50

benjaminapetersen mentioned this pull request Sep 19, 2017

Notification Drawer Test #2118

Closed

openshift-bot merged commit 1ceb0ff into openshift:master Sep 19, 2017

benjaminapetersen deleted the bpeterse/notifications-to-notification-drawer branch September 19, 2017 21:51

Custom notifications in notification drawer #2001

Custom notifications in notification drawer #2001

Conversation

benjaminapetersen commented Sep 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjaminapetersen commented Sep 6, 2017

dtaylor113 commented Sep 6, 2017

dtaylor113 commented Sep 6, 2017

benjaminapetersen commented Sep 6, 2017

benjaminapetersen commented Sep 6, 2017

benjaminapetersen commented Sep 6, 2017

benjaminapetersen commented Sep 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spadgett commented Sep 6, 2017

benjaminapetersen commented Sep 6, 2017 • edited Loading

ncameronbritt commented Sep 6, 2017

benjaminapetersen commented Sep 6, 2017

ncameronbritt commented Sep 6, 2017

serenamarie125 commented Sep 6, 2017

spadgett commented Sep 6, 2017

serenamarie125 commented Sep 7, 2017

serenamarie125 commented Sep 7, 2017

dtaylor113 commented Sep 18, 2017

benjaminapetersen commented Sep 18, 2017

benjaminapetersen commented Sep 19, 2017 • edited Loading

spadgett commented Sep 19, 2017

benjaminapetersen commented Sep 19, 2017

benjaminapetersen commented Sep 19, 2017

benjaminapetersen commented Sep 19, 2017

benjaminapetersen commented Sep 19, 2017

spadgett commented Sep 19, 2017

benjaminapetersen commented Sep 19, 2017

openshift-bot commented Sep 19, 2017

openshift-bot commented Sep 19, 2017

openshift-bot commented Sep 19, 2017 • edited Loading

openshift-bot commented Sep 19, 2017

benjaminapetersen commented Sep 2, 2017 •

edited

Loading

benjaminapetersen commented Sep 6, 2017 •

edited

Loading

benjaminapetersen commented Sep 19, 2017 •

edited

Loading

openshift-bot commented Sep 19, 2017 •

edited

Loading