Add notification bar component. (Fixes #383) #431

amychurchwell · 2019-07-18T02:15:03Z

Description

Adds the notification bar component used on /thanks and /whatsnew

I have documented this change in the design system.
I have recorded this change in CHANGELOG.md.

Issue

#383
mozilla/bedrock#7468

Testing

https://demo2--mozilla-protocol.netlify.com/patterns/molecules/notification-bar.html

Supports different styles:
- Success
- Warning
- Error
- Click
Dismiss is optional.

amychurchwell · 2019-07-18T02:17:27Z

Should we support a standard set cookie for components that utilize a dismiss function like this in protocol? I'm not sure if this should be baked in on a per-component basis or a reusable token(?) of some kind.

alexgibson · 2019-07-18T06:52:46Z

My personal take here is that Protocol shouldn’t use or require setting any form of cookies in its code (that’s probably a bigger discussion that we’ve yet to have). Providing a callback when the notification is dismissed should be enough so that consumers (i.e bedrock) can then use cookies if required. It can also be useful for things like GA, which can differ from site to site.

It might be nice to have a JavaScript API that enables showing the notification itself too, vs a script that does it automatically on load? Also, tests!

craigcook · 2019-07-20T00:55:02Z

A few drive-by opinions, not a review because I know it's WIP.

It probably shouldn't be an h1. The original one on the whatsnew page was because it's arguably the most important content on that particular page, but as a general-purpose notification the example should be a plain old p, or maybe a div or aside with a p for the content.

We should move the close button to the left in RTL.

And this is more of a discussion for the designers, but I'm not sure the "click" variant should even exist. It appears in the brand guide but I think it was just an example of "use blue for actions," it wasn't a fully thought-through component. If there's an action to be taken it shouldn't take the form of a big pseudo-button with its own little dismiss button inside it. We should get some more opinions on that.

I also think we might need a neutral notification for messages that aren't warnings, errors, or successes, just general notices. Maybe with a light gray background, or white with a blue or purple border?

amychurchwell · 2019-07-25T23:34:56Z

@stephaniehobson If you have a moment tomorrow, would you mind taking a look at this? I've refactored the JS a bit but I'm struggling with the unit test portion (or perhaps the JS needs to be refactored more?) 😔

amychurchwell · 2019-07-29T17:41:53Z

@alexgibson would you mind taking a look at this tomorrow? (only if you have the time, is not a p1)
The JS isn't quite right and writing unit tests has been a little blocked.

alexgibson

I took a look at the tests and left a few comment / suggestions.

Happy to review this more fully when you think it's ready. Looking great!

src/patterns/molecules/notification-bar/notification-bar.hbs

tests/unit/protocol-notification-bar.js

src/assets/js/protocol/protocol-notification-bar.js

amychurchwell · 2019-07-31T20:55:13Z

@alexgibson thanks for the feedback, Alex! I tidied things up and everything seems to be working as it should. I only have two unit tests for opening & closing a notification, I wasn't sure what else to include?
Also, in it's current form, does this component feel ready to use on whatsnew pages? (I'm not quite sure if the way I've written the init of this component requires the use of a "trigger", whereas on wnp the bar is static.)

alexgibson

A few comments / suggestions, and might need a small refactor to work on the browsers bedrock needs to support. But overall I think this is looking teriffic, nice work!

package-lock.json

src/assets/js/protocol/protocol-notification-bar.js

src/patterns/molecules/notification-bar/notification-bar.hbs

alexgibson · 2019-08-01T14:18:25Z

Styling on smaller viewports needs a little work:

src/assets/sass/protocol/components/_notification-bar.scss

alexgibson · 2019-08-01T14:23:36Z

Also, in it's current form, does this component feel ready to use on whatsnew pages? (I'm not quite sure if the way I've written the init of this component requires the use of a "trigger", whereas on wnp the bar is static.)

I think the way you've written it feels right, but we should also consider that on some pages we may not require the use of JavaScript at all. For example, this bar is already on /firefox/download/thanks/ (minus the close button). As long as we have the option to include it statically on pages without needing JavaScript, then what you're added here is a nice progressive enhancement, should we ever need it 👍

alexgibson · 2019-08-01T14:24:32Z

Oh, the bar on /download/thanks/ also includes a link:

I'm guessing this should also be factored into the component design?

alexgibson

Took a second pass, looking good! The styles for small screens (i.e. mobile) could use a few tweaks, but the content is resizing well for all other breakpoints 👍

src/assets/js/protocol/protocol-notification-bar.js

alexgibson · 2019-08-05T15:33:06Z

src/assets/js/protocol/protocol-notification-bar.js

+            // add title to notification
+            notification.appendChild(notificationTitle);
+        }else{
+            console.log('Notification banner requires title parameter.'); // eslint-disable-line no-console


I don't think we should leave console.log() in as production code, since it can cause errors in older browsers unless dev tools are open.

agreed! will fix that up tomorrow.

tests/unit/protocol-notification-bar.js

src/assets/sass/protocol/components/_notification-bar.scss

alexgibson · 2019-08-07T08:11:05Z

One other thought: we should consider a11y when a notification is programatically opened using JS. What should happen: should focus move to the notification title when it is displayed? Should that be required, or optional? Do we need any additional ARIA information?

amychurchwell · 2019-08-07T23:38:35Z

Tests are passing locally, I'm not sure why travis is failing. Also, I'm not entirely sure what additional ARIA information may be useful here. @alexgibson

alexgibson

One suggested change on why the tests are failing, but otherwise looks great.

I'd suggest we file separate issues for followups on this component:

Improve a11y when a notification opens.
Allow a notification to contain a link (such as the one on /download/thanks/)

Does that sound about right?

alexgibson · 2019-08-08T08:03:07Z

tests/unit/protocol-notification-bar.js

+            expect(notification).toBeTruthy();
+        });
+
+        it('closes as expected', function() {


Here you've created an ordered dependency on the "open" test needing to run before the "close" test, which will lead to intermittent failures. Tests run in a random order and not sequentially (hence why this passes sometimes and not at other times, like in CI).

I'd suggest treating each test as it's own distinct piece of logic. Before each test runs you should create the notification, and after each test finishes you should remove the notification. That way, tests should pass regardless of the order they are run.

alexgibson

One last suggestion on tidying up after the tests, but it's a nit. This looks great, nice work!

alexgibson · 2019-08-08T16:53:20Z

tests/unit/protocol-notification-bar.js

+
+    'use strict';
+
+    beforeEach(function () {


Nit: Suggest adding an afterEach below here to remove the button after each test also?

Ah! Yes! Good catch!

amychurchwell force-pushed the notification-banner branch from d10e531 to dccb251 Compare July 19, 2019 19:05

amychurchwell added the WIP 🚧 Work In Progress label Jul 19, 2019

amychurchwell force-pushed the notification-banner branch from dccb251 to c90dc5a Compare July 19, 2019 20:54

amychurchwell mentioned this pull request Jul 22, 2019

Add animation to notification bar component #440

Closed

alexgibson requested changes Jul 30, 2019

View reviewed changes

src/patterns/molecules/notification-bar/notification-bar.hbs Outdated Show resolved Hide resolved

tests/unit/protocol-notification-bar.js Outdated Show resolved Hide resolved

src/assets/js/protocol/protocol-notification-bar.js Outdated Show resolved Hide resolved

amychurchwell added Needs:Review 👋 Ready for Developer Review and removed WIP 🚧 Work In Progress labels Jul 31, 2019

alexgibson requested changes Aug 1, 2019

View reviewed changes

alexgibson reviewed Aug 1, 2019

View reviewed changes

src/assets/sass/protocol/components/_notification-bar.scss Outdated Show resolved Hide resolved

amychurchwell added 2 commits August 1, 2019 10:30

Add notification bar component. (Fixes mozilla#383)

75897e3

Update JS, responsive styles, and docs.

093b6ab

amychurchwell force-pushed the notification-banner branch from 010562b to 093b6ab Compare August 2, 2019 22:01

alexgibson reviewed Aug 5, 2019

View reviewed changes

Update styles, TODO: add DOM tests + tidy JS error catch

f59ccc5

amychurchwell added WIP 🚧 Work In Progress and removed Needs:Review 👋 Ready for Developer Review labels Aug 6, 2019

Adjust styles & add tests

ceafe7d

amychurchwell added 2 commits August 7, 2019 13:42

Remove console.log & adjust styles

b775220

Adjust tests

c075934

alexgibson requested changes Aug 8, 2019

View reviewed changes

amychurchwell mentioned this pull request Aug 8, 2019

Notification Bar: improve a11y when notification opens. #459

Open

Remove banner after each unit test

74513e5

amychurchwell mentioned this pull request Aug 8, 2019

Notification Bar: Add optional link cta #460

Closed

alexgibson approved these changes Aug 8, 2019

View reviewed changes

Remove trigger button after each unit test

560c0e1

amychurchwell removed the WIP 🚧 Work In Progress label Aug 8, 2019

amychurchwell merged commit 817f3c3 into mozilla:master Aug 8, 2019

amychurchwell deleted the notification-banner branch August 8, 2019 17:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add notification bar component. (Fixes #383) #431

Add notification bar component. (Fixes #383) #431

amychurchwell commented Jul 18, 2019

amychurchwell commented Jul 18, 2019

alexgibson commented Jul 18, 2019 •

edited

Loading

craigcook commented Jul 20, 2019

amychurchwell commented Jul 25, 2019

amychurchwell commented Jul 29, 2019

alexgibson left a comment

amychurchwell commented Jul 31, 2019

alexgibson left a comment

alexgibson commented Aug 1, 2019

alexgibson commented Aug 1, 2019 •

edited

Loading

alexgibson commented Aug 1, 2019

alexgibson left a comment

alexgibson Aug 5, 2019

amychurchwell Aug 7, 2019

alexgibson commented Aug 7, 2019

amychurchwell commented Aug 7, 2019

alexgibson left a comment

alexgibson Aug 8, 2019

alexgibson left a comment

alexgibson Aug 8, 2019

amychurchwell Aug 8, 2019

Add notification bar component. (Fixes #383) #431

Add notification bar component. (Fixes #383) #431

Conversation

amychurchwell commented Jul 18, 2019

Description

Issue

Testing

amychurchwell commented Jul 18, 2019

alexgibson commented Jul 18, 2019 • edited Loading

craigcook commented Jul 20, 2019

amychurchwell commented Jul 25, 2019

amychurchwell commented Jul 29, 2019

alexgibson left a comment

Choose a reason for hiding this comment

amychurchwell commented Jul 31, 2019

alexgibson left a comment

Choose a reason for hiding this comment

alexgibson commented Aug 1, 2019

alexgibson commented Aug 1, 2019 • edited Loading

alexgibson commented Aug 1, 2019

alexgibson left a comment

Choose a reason for hiding this comment

alexgibson Aug 5, 2019

Choose a reason for hiding this comment

amychurchwell Aug 7, 2019

Choose a reason for hiding this comment

alexgibson commented Aug 7, 2019

amychurchwell commented Aug 7, 2019

alexgibson left a comment

Choose a reason for hiding this comment

alexgibson Aug 8, 2019

Choose a reason for hiding this comment

alexgibson left a comment

Choose a reason for hiding this comment

alexgibson Aug 8, 2019

Choose a reason for hiding this comment

amychurchwell Aug 8, 2019

Choose a reason for hiding this comment

alexgibson commented Jul 18, 2019 •

edited

Loading

alexgibson commented Aug 1, 2019 •

edited

Loading