Skip to content
This repository has been archived by the owner on May 4, 2022. It is now read-only.

Notifications may be non-dismissible #521

Merged
merged 9 commits into from
Sep 13, 2017

Conversation

thevoiceofzeke
Copy link
Contributor

@thevoiceofzeke thevoiceofzeke commented Sep 12, 2017

MUMUP-3021: "As a user, I would like MyUW notifications to sometimes be non-dismissable, so that I am reminded more often and can't accidentally dismiss a notification that is attempting to notify me of a critical action I need to take."

In this PR:

  • Add support for notDismissible attribute in messages data model
  • Tweak notifications/announcements menu UI
    • Priority flag less disruptive of UI, but still noticeable
    • Announcement titles look like notification titles
    • Description text slightly lighter color
    • Dismiss button centered in announcement menu items
  • If addToHome action has already been taken, disable button
  • Update messaging configuration doc

Screenshots

notDismissible notification

screen shot 2017-09-12 at 2 21 32 pm

High-priority UI tweak

screen shot 2017-09-12 at 2 12 39 pm

Announcement item UI tweak

screen shot 2017-09-12 at 2 22 10 pm


Contributor License Agreement adherence:

Copy link
Contributor

@ChristianMurphy ChristianMurphy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @thevoiceofzeke! 💯

<md-icon aria-label="important:" flex="10">priority_high</md-icon>
<span flex="90"> {{ notification.title }}</span>
<div class="menu-item__high-priority" ng-if="notification.priority">
<md-icon class="md-warn" aria-label="important:">priority_high</md-icon>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the <span>High priority</span>, maybe this md-icon can be considered decorative and aria-hidden? Or aria-labelledby the span?

<div layout="row" layout-align="start center" ng-if="notification.priority">
<md-icon aria-label="important:" flex="10">priority_high</md-icon>
<span flex="90"> {{ notification.title }}</span>
<div class="menu-item__high-priority" ng-if="notification.priority">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe should ng-if on notification.priority having the actual value "high", so that if we would in the future invent other priority values this high priority treatment wouldn't trigger on them.

@@ -86,6 +91,7 @@
"expireDate": "2017-08-15",
"featureImageUrl": "img/the-cow.png",
"priority": "high",
"notDismissible": false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not with the negative in the name of the property. Maybe "userDismissible": true, ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My thinking here was that the default case should be that dismissible is true, since the vast majority of notifications are not important enough to linger. The attribute should be optional and should only be set when something's important enough to be non-dismissible -- otherwise folks shouldn't have to bother with it. I agree that the negative property is a bad look. What if we used something to reflect non-dismissibility, like "permanent" or "sticky"?

@@ -54,6 +55,7 @@ point uw-frame to your desired feed.
- **expireDate**: *(optional)* Accepts a simple ISO date, including time (as pictured). This is used to stop displaying a message at a certain day/time.
- **featureImageUrl**: *(optional)* Used by popup announcements and announcements on the Features page.
- **priority**: Set to "high" if you want the message to be displayed with higher visibility (i.e. As a priority notification or popup announcement, respectively).
- **isSticky**: *(experimental)* Set to true if you want to disallow users from dismissing a notification. This should only be used for truly critical messages.
Copy link
Contributor

@apetro apetro Sep 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider documenting default behavior. Maybe "By default users can dismiss notifications. Set to true to disallow user dismissing for this notification."

"This should only be used for truly critical messages.": I'm not sure we know that this is the guidance yet. Stickiness may be less about criticality and more about does the notification dismiss itself when the underlying business process is fulfilled. If we can tell that you no longer have a library fine preventing your graduating, we should yank the notification for you when you pay the fine. Maybe how well we can automate this for the user drives how much dismissing the notification work we ought to expose to the user.

(Notifications are like a to do list where the items check themselves off when you do them?)

We might get to guidance that whether user dismissing should be disabled depends only on the criticality of the message. I think the point of the prospective user research is to figure this out.

@@ -8,7 +8,7 @@
layout-align="start center"
layout-xs="column"
layout-align-xs="center start">
<span><md-icon ng-if="notification.priority">priority_high</md-icon> {{ notification.title }}</span>
<span><md-icon ng-if="notification.priority == 'high'">priority_high</md-icon> {{ notification.title }}</span>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do non-visual users understand that the notification is a priority notification?

@thevoiceofzeke thevoiceofzeke merged commit 22b86ba into uPortal-Attic:master Sep 13, 2017
@davidmsibley davidmsibley added this to the 5.2.0 milestone Sep 20, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants