Notifications drawer #1326
Notifications drawer #1326
Conversation
|
More thoughts:
|
| // eliminates the callback form the list | ||
| // closes the watch if it is no longer needed | ||
| unsubscribe: function(handler) { | ||
| subscriptions[handler.projectName].callbacks.splice(handler.cbIndex, 1); |
There was a problem hiding this comment.
This .splice() mutates the array & throws off all the indices, so it wont work for the following .unsubscribe() call. Per @jwforres using something like $.Callbacks() will take care of this problem.
|
Per google doc, check to see if we have the following events available:
Events we are currently choosing to ignore:
Working on mapping the events I can trigger into something easy to parse in this gist, so we can clarify what is still needed. |
|
Updated drawer headings to match the 3 primary in the menu, sorting events appropriately (still working on filtering):
|
benjaminapetersen
force-pushed
the
notifications
branch
from
2fce7bb
to
70ab5ba
Compare
|
@jwforres keeping track of the types of Events I've been able to generate here, per our last discussion. gist with events |
|
@benjaminapetersen You should grab the reason codes, which is probably the best way to identify the events |
|
So what I would like to see is in the google doc, for each of the events we
said we want, either list the actual API event we can use to meet the
requirement, or say that there isnt one, and use the google doc as the
canonical list for what we do / dont have covered at the API level.
…On Wed, Mar 15, 2017 at 2:48 PM, Ben Petersen ***@***.***> wrote:
@jwforres <https://github.com/jwforres> keeping track of the types of
Events I've been able to generate here, per our last discussion. gist
with events
<https://gist.github.com/benjaminapetersen/93b34979092fdaec04f29822b09c96ea>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1326 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABZk7W7B7fMx8S_l8Wa1bjxIceCzv79jks5rmDKZgaJpZM4MW_4B>
.
|
|
Will do, I have a much bigger list going now after going fishing in the Go code to find |
|
Tracked what I found in the doc, top has the things we were looking for, bottom has the full list. |
openshift-bot
added
the
needs-rebase
benjaminapetersen
force-pushed
the
notifications
branch
from
808543f
to
0afc96f
Compare
openshift-bot
added
needs-rebase
|
@benjaminapetersen this is likely early for me to comment, but just want to make sure that the badge will be blue ( the upcoming PF update should do that ), and verify that you are aware that will not be using #s inside the badge. Tagging @beanh66 as well |
|
@serenamarie125 yup, thanks! As far as I understand this is where things are currently: @jeff-phillips-18 is keeping me updated on some changes he has in progress. |
benjaminapetersen
force-pushed
the
notifications
branch
from
0afc96f
to
56f1287
Compare
openshift-bot
added
needs-rebase
benjaminapetersen
force-pushed
the
notifications
branch
from
56f1287
to
1fc4563
Compare
openshift-bot
removed
the
needs-rebase
benjaminapetersen
force-pushed
the
notifications
branch
2 times, most recently
from
ac16065
to
52a0764
Compare
|
@spadgett this pass is prob ready to start reviewing. My API server was crashing & giving me trouble over the weekend, so I haven't included the work to include our internal Notifications quite yet. Working on diagnosing that issue this morning. |
|
Depends on web-common PR 138 for some fixes to notification-drawer |
| // BuildConfig | ||
| // | ||
| // Deployment | ||
| Failed: true, |
There was a problem hiding this comment.
Some of these are so general, I'd rather pair it to a specific kind. Although API groups make it a little harder. Maybe something like
Failed: {
resources: [{ group: 'apps', resource: 'deployments' }],
drawer: true,
toast: true
}There was a problem hiding this comment.
ok sounds good. I can definitely iterate on this.
| // | ||
| // PodAutoscaler | ||
| SuccessfulRescale: true, | ||
| FailedRescale: true, |
There was a problem hiding this comment.
There are some events for when the autoscaler can't get metrics. We should show those.
There was a problem hiding this comment.
I was able to find the following related to autoscalers:
PodAutoscaler
Normal
DesiredReplicasComputed
DesiredReplicasComputedCustomMetric
SuccessfulRescale
Warning
FailedRescale
There was a problem hiding this comment.
Nevermind, I'm thinking of a condition on the HPA object, not an event.
| '$rootScope', | ||
| 'Constants', | ||
| 'DataService', | ||
| function($routeParams, $rootScope, Constants, DataService) { |
There was a problem hiding this comment.
Nit: I'd like to keep a consistent style for declaring components. In other places, we don't use an inline function.
app/scripts/constants.js
Outdated
| @@ -84,7 +84,11 @@ angular.extend(window.OPENSHIFT_CONSTANTS, { | |||
|
|
|||
| // This blacklist hides certain kinds from the "Other Resources" page because they are unpersisted, disallowed for most end users, or not supported by openshift but exist in kubernetes | |||
| AVAILABLE_KINDS_BLACKLIST: [], | |||
|
|
|||
| // Kill switches for features that are not in tech-preview | |||
| FEATURE_FLAGS: { | |||
There was a problem hiding this comment.
I wish we had done this from the start, but the other flags are currently DISABLE_<FEATURE_NAME>. A few of those are documented. Trying to decide what's best for consistency.
There was a problem hiding this comment.
I'll change for consistency now. I prob prefer this way for clarity, but def isn't a huge deal.
| counter.$onInit = function() { | ||
| reset(); | ||
| rootScopeWatches.push($rootScope.$on("$routeChangeSuccess", function () { | ||
| reset(); |
There was a problem hiding this comment.
Let's only reset if the project route param changes.
| class="notification-counter notification-counter-offset" | ||
| row main-axis="center" cross-axis="center"> | ||
| <span><!--{{countDisplay}}--></span> | ||
| </div> |
There was a problem hiding this comment.
Looks like the content is commented out?
There was a problem hiding this comment.
Yup, the latest designs indicate we won't show count. We just show the indicator.
There was a problem hiding this comment.
OK, mind removing out the commented out expression (and any unnecessary markup)?
We should add some screenreader text as well per my comment below.
| row main-axis="center" cross-axis="center"> | ||
| <span><!--{{countDisplay}}--></span> | ||
| </div> | ||
| <span class="fa fa-bell" aria-hidden="true"></span> |
There was a problem hiding this comment.
Needs some sr-only text somewhere here.
There was a problem hiding this comment.
This strays from the patternfly designs adding indicator colors and such. Why not follow the example from patternfly:
<li class="drawer-pf-trigger">
<a class="nav-item-iconic">
<span class="fa fa-bell" title="Notifications"></span>
<span class="badge"> </span>
</a>
</li>
There was a problem hiding this comment.
@jeff-phillips-18 Agree, but I see some problems with the example as well. Can we fix in Patternfly?
aneeds anhrefor you can't access it by the keyboard or screen readerfa-bellshould havearia-hidden="true"- Needs some
sr-onlytext for screen readers
There was a problem hiding this comment.
yeah, we can fix the example, but it is still just example so would need to be done here as well. I tried replacing and the biggest issue is the navbar-alt-pf styling.
Adding the following fixes it up and allows the removal of the other counter settings:
_notifications-counter.html (yeah needs some of the above fixes):
<li class="drawer-pf-trigger" ng-if="!$ctrl.hide">
<a href="" class="nav-item-iconic" ng-click="$ctrl.onClick()">
<span class="fa fa-bell" title="Notifications" aria-hidden="true"></span>
<span ng-if="$ctrl.showNewNotificationIndicator" class="badge"> </span>
</a>
</li>
_notifications.less:
.navbar-pf-alt .nav .nav-item-iconic .badge {
font-size: 9px;
margin: -15px 0 0 -12px;
min-width: 10px;
min-height: 10px;
}
There was a problem hiding this comment.
@jeff-phillips-18 thanks! I'll update to bring in line w/patternfly.
| @@ -0,0 +1,8 @@ | |||
| <a | |||
| ng-if="$ctrl.customScope.hasUnread(notificationGroup)" | |||
There was a problem hiding this comment.
Would rather calculate this only when things change instead of in each digest loop.
| @@ -0,0 +1,8 @@ | |||
| <a | |||
| ng-if="$ctrl.customScope.hasUnread(notificationGroup)" | |||
| class="btn btn-link btn-block" | |||
There was a problem hiding this comment.
Confusing why it's an a but we're using the btn-link style
| class="btn btn-link btn-block" | ||
| role="button" | ||
| ng-click="$ctrl.customScope.clearAll(notificationGroup)"> | ||
| <span class="pficon pficon-close"></span> |
|
Nixed that last comment, after a rebuild it seems that bug does not exist. |
|
@beanh66 I believe you made a previous comment expecting the drawer to be full width @ mobile. Is that correct? Quick screenshot for reference:
I tend to agree & think it might be a better use of screen real estate if it became full width @ mobile & filled whatever space was available. |
|
@benjaminapetersen Could you comment in the patternfly PR for this? |
|
@jeff-phillips-18 yup, just submitted my review. |
benjaminapetersen
force-pushed
the
notifications
branch
2 times, most recently
from
03fac47
to
3107661
Compare
openshift-bot
removed
the
needs-rebase
|
One thing to note, the counter is still in the old nav in this PR, which means it re-renders on every view. The "new" indicator disappears. This will not happen in our new design by @sg00dwin as the header is persistent (the counter will not lose its internal state). |
| rootScopeWatches.push($rootScope.$on("$routeChangeSuccess", function (evt, next, current) { | ||
| if(projectChanged(next, current)) { | ||
| drawer.customScope.projectName = $routeParams.project; | ||
| reset(); |
There was a problem hiding this comment.
Can you check that reset() is not called twice when first loading the page?
There was a problem hiding this comment.
Yup, it is only called once.
| if(DISABLE_GLOBAL_EVENT_WATCH || LIMIT_WATCHES) { | ||
| return; | ||
| } | ||
| if (AuthService.isLoggedIn()) { |
There was a problem hiding this comment.
I know I suggested this, but I'd like to check if we can get away with only starting the watch $onRouteChangeSuccess and if this is still needed in that case?
|
The expand and close links in the drawer titlebar aren't accessible because the
|
|
The icons in the drawer should also have
|
- Show events that pass the white list in OPENSHIFT_CONSTANTS.EVENTS_TO_SHOW
- Internal Notifications coming in next iteration
- Add patternfly.notifications
- add notification counter to top bar
- add notification event service
- add drawer-wrapper as a proxy to pf-notification-drawer
- need our own controller for filtering, etc
- Update event system to use $rootScope.$emit rather than a custom service
- add sessionStorage cache to keep track of read/unread state
- Debounce watch in notification service based on work recently done in DataService to support
- Update web-common to 0.0.43, includes DataService.watch() flag
- Add FEATURE_FLAG.global_event_watch_for_notification_drawer
- kill switch in case watching events is too expensive
- would still allow internal notifications to appear in drawer
- Add link to events page, update index.html
- Migrate EVENTS_TO_SHOW map out of EventsService into Constants
- Add naviateEventInvolvedObjectURL filter for drawer & events
- Update Events service to track read, cleared, namespace SessionStorage, etc
- Animate w/a fade-out when a notification is cleared to ensure user notices the subtle change
- Add BrowserStorage service
- auto namespace openshift- on all Local & Session storage keys
- Add links to objects in event-sidebar
- Disabled in IE & Edge due to insufficient watches
benjaminapetersen
force-pushed
the
notifications
branch
from
3107661
to
2fb7ef6
Compare
|
Evaluated for origin web console test up to 2fb7ef6 |
| counter.hide = true; | ||
| return; | ||
| } | ||
| initWatches(); |
There was a problem hiding this comment.
@spadgett the Auth items removed.
| aria-hidden="true" | ||
| class="pull-left" | ||
| ng-class="$ctrl.customScope.getNotficationStatusIconClass(notification.event)"></span> | ||
| <span class="sr-only">{{notification.event.type}}</span> |
There was a problem hiding this comment.
@spadgett the sr-only for the icon.
| @@ -7,15 +7,15 @@ const projectHelpers = require('../helpers/project.js'); | |||
| let goToAddToProjectPage = (projectName) => { | |||
| let uri = 'project/' + projectName + '/create'; | |||
| h.goToPage(uri); | |||
| expect(element(by.cssContainingText('h1', "Create Using Your Code")).isPresent()).toBe(true); | |||
| expect(element(by.cssContainingText('h1', "Create Using a Template")).isPresent()).toBe(true); | |||
| expect(element(by.cssContainingText('.middle-container h1', "Create Using Your Code")).isPresent()).toBe(true); | |||
|
[merge] |
|
Origin Web Console Test Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_web_console/94/) (Base Commit: cf0278b) (PR Branch Commit: 2fb7ef6) |
|
Flake #1684 Error: Timed out waiting for the WebDriver server at http://172.18.0.16:39874/wd/hub |
| @@ -0,0 +1,67 @@ | |||
| // fixes positioning that doesn't quite work w/core patternfly styles | |||
| .navbar-pf-alt .nav .nav-item-iconic .badge { | |||
|
Origin Web Console Merge Results: Waiting: You are in the build queue at position: 1 |
|
[merge] |
|
Evaluated for origin web console merge up to 2fb7ef6 |
https://trello.com/c/o82aiHhh
WIP Prototype of the notification drawer.
TODO:
Collapse history of objects, ifmongodb-1-deploy,mongodb-2-deployandmongodb-3-delpoyall have events, only show what is current & relevant (mongodb-3-deploy).click to dismiss notifications, if all dismissed for a group, remove group.Designs for reference:
@jwforres @spadgett opening up for conversation.