Add notification component #348

afercia · 2017-10-13T16:14:43Z

Summary

This PR can be summarized in the following changelog entry:

Added a Notification component.

Relevant technical choices:

To discuss: how to pass the notification text, since it may contain HTML
the Heading level can be set via props: this way the component can be rendered anywhere and the heading adjusted to the existing heading hierarchy

Test instructions

Use the code below in ContentAnalysis.
Play a bit changing the props, removing imageSrc, removing isDismissable, etc.
Test in responsive view.

import React from "react";
import styled from "styled-components";

import Notification from "../../../basic/Notification";

export const ContentAnalysisContainer = styled.div`
	min-height: 700px;
	width: 80%;
	margin: 0 auto;
`;

/**
 * Returns the ContentAnalysis component.
 *
 * @returns {ReactElement} The ContentAnalysis component.
 */
export default function ContentAnalysis() {
	let html = "Get started with our <a href=\"#blabla\">Configuration Wizard!</a>";

	return <ContentAnalysisContainer>
		<Notification
			title="New User?"
			html={ html }
			isDismissable={ true }
			// Images to test with:
			// http://via.placeholder.com/70x70
			// https://user-images.githubusercontent.com/6073772/30917585-a4ae3da0-a39c-11e7-89fa-e60840707fe1.png
			imageSrc="https://user-images.githubusercontent.com/6073772/30917585-a4ae3da0-a39c-11e7-89fa-e60840707fe1.png"
			imageWidth="40px"
			imageHeight="40px"
			onClick={ () => {
				console.log( "clicked!" );
			} }
		/>
	</ContentAnalysisContainer>;
}

Screenshots:

Quick testing integration in WordPress (just pasting the rendered HTML and stylesheets in the DOM):

Fixes #347

IreneStr · 2017-10-16T06:44:30Z

@afercia Before my code review, one visual thing: I think the box shadow of the component should be less pronounced (matching with the metabox below).

afercia · 2017-10-16T09:10:31Z

@IreneStr yep I've noticed there's a difference in the box-shadow, however the current box-shadow is already used in the HelpCenter and matches the one in My Yoast. It's just the default one used for the Paper component. Since this is a re-usable component, I'd say its styling should be consistent everywhere so there are 2 options:

we change all the box-shadow in My Yoast
we change all the box-shadow in the plugin
we make the Paper component have a prop to switch to a different box-shadow style (please, no 😬 )

/cc @hedgefield

Values:
Paper box-shadow: 0 2px 0.8em 0 rgba(0,0,0,0.2)
Alerts box-shadow: box-shadow: 0 1px 1px rgba(0, 0, 0, 0.04); (they also have a 1px border)

One more small difference is that this notification has a white background. However, the meta boxes have a light gray background and only the actual notifications inside the meta boxes have a white background:

hedgefield · 2017-10-16T09:26:02Z

If we change the shadow in the plugin, will it still have the border? That's what makes it match the Wordpress UI. If so, go ahead and replace the shadow values with the ones from My Yoast so they're they same everywhere 👍

As for the color, Wordpress-native notifications are #fff, this one is similar, so keep it white :)

IreneStr · 2017-10-16T09:47:01Z

@afercia @hedgefield One other concern: in mobile view, the notification takes up about 1/3 of the screen. Would it be a better idea to remove the image on mobile? Or maybe move the dismiss button to the top right corner to clear more space for the text?

hedgefield · 2017-10-16T09:51:48Z

I agree with both suggestions, try that. There should be enough space next to the heading for the dismiss button up there (and to not have to wrap the heading - unless the heading is part of the same text area as the notification text itself...)

IreneStr · 2017-10-16T07:32:15Z

style-guide/svg/times.svg

@@ -0,0 +1,3 @@
+<svg width="1792" height="1792" viewBox="0 0 1792 1792" xmlns="http://www.w3.org/2000/svg">


Filename is not the most descriptive for the way we're using it. Maybe 'cross' is a better name.

This is the same name of the icon used on My Yoast, see client/src/icons/times.svg. I think we should be consistent and keep it as is or change it also on My Yoast.

Ah, okay. Let's keep it this way 👌

It's a fun one, I can also never find it in fontawesome because of that name 😄 it doesn't really match how the icon is being used on the web today. But while we're sourcing from fontawesome we best keep it the same yeah.

IreneStr · 2017-10-16T07:32:24Z

style-guide/svg/index.js

@@ -5,3 +5,4 @@ export { default as angleRight } from "./angle-right.svg";
 export { default as angleUp } from "./angle-up.svg";
 export { default as angleDown } from "./angle-down.svg";
 export { default as questionCircle } from "./question-circle.svg";
+export { default as times } from "./times.svg";


IreneStr · 2017-10-16T08:52:14Z

composites/basic/Notification.js

+const messages = defineMessages( {
+ buttonAriaLabel: {
+ id: "notification.buttonAriaLabel",
+ defaultMessage: "Dismiss this notice",


I've discussed semantics with @jcomack. Please change to 'notification' ('notice' = warning)

I've used this text because it's the standard text used in WordPress: Dismiss this notice.
No objections in changing it, however users will hear different text when navigating through the interface, sometimes Dismiss this notice. and sometimes Dismiss this notification. 🙂 Not sure hte change will add a great value, at the cost of some potential confusion.
/cc @jcomack

Let's keep this consistent with WP: notice

IreneStr · 2017-10-16T09:58:29Z

composites/basic/Notification.js

+const NotificationImage = styled.img`
+ flex: 0 0 ${ props => props.imageWidth ? props.imageWidth : "auto" };
+ height: ${ props => props.imageHeight ? props.imageHeight : "auto" };
+ margin-right: 18px;


@hedgefield prefers 16px to fit the 8px grid

Same as above: in the provided mockup, the distance is 20px. I've use 18px to compensate the font metrics. If you measure the distance from the rendered image and the rendered title, it's 20px.
Are we sure we want to change this? The meta boxes below use a 20px padding.

To clarify: font metrics and font rendering engines matter. If we use a distance (whether it's padding or margin) between the image and the text, this applies to the text container. The actual text is rendered with a few pixel distance from its container, and that's normal on the web, it just works this way. It also depends on the actual font face used and even from the shape of the first letter:

Setting the exact same value on the left and on the right, will actually result in an uneven perceived spacing.

That's interesting, so 18px is actually 20px visually? I hadn't heard about that before. Is this compensated consistently throughout our existing UI? If so, okay, otherwise, I'd prefer coding consistency over a small visual inconsistency.

IreneStr · 2017-10-16T10:01:16Z

composites/basic/Notification.js

+const NotificationContainer = styled.div`
+ display: flex;
+ align-items: center;
+ padding: 20px;


24px in desktop, 16px in mobile view

to fit the 8px grid

This change wouldn't match the provided design, see #347. I've measured the padding in the mockup, and it's 20 px, I guess also to match the padding of the notifications in the meta boxes below.
Are we sure we want to change this?

@hedgefield

The margins in the mockup are an approximation, they haven't been measured out, I'll keep that in mind next time. I would prefer they all adhere to the 24/16 @IreneStr proposed.

afercia · 2017-10-16T12:37:15Z

Latest commits addressed the responsive view, see screenshot below. Pending feedback about the padding etc.

afercia · 2017-10-17T07:41:38Z

If we change the shadow in the plugin, will it still have the border? That's what makes it match the Wordpress UI. If so, go ahead and replace the shadow values with the ones from My Yoast so they're they same everywhere

No, because the My Yoast style used for the "Paper" in the Help Center and in the new notification doesn't have a border. If we want the styles to match, the border should be removed, or the My Yoast style changed.

Re: containers/notices styling, here's the current situation:

WordPress meta boxes:
border: 1px solid #e5e5e5;
background: #fff;
box-shadow: 0 1px 1px rgba(0,0,0,0.04);

Yoast Container (the one used for the alerts in the SEO dashboard)
border: 1px solid #e5e5e5;
background-color: #fdfdfd;
box-shadow: 0 1px 1px rgba(0, 0, 0, 0.04);

They match except for the background. Our background is a light gray because we're using notices with a white background inside these containers. Aside; I'm not sure I like the white on (almost) white effect 🙂

WordPress notices
background: #fff;
box-shadow: 0 1px 1px 0 rgba( 0, 0, 0, 0.1 );

yoast-alert
background: #fff;
box-shadow: 0 1px 2px rgba(0, 0, 0, 0.2);

The box-shadow doesn't match.

Help Center "paper" and new "notification" component
box-shadow: 0 2px 0.8em 0 rgba(0,0,0,0.2);
background-color: #fff;

This box-shadow is completely different because it uses the "My Yoast" style. As I see it, there are 2 options:

keep the reusable components style unchanged, whether they're used on My Yoast, on WordPress or anywhere else
differentiate the style of the components depending if they're used on WordPress or other places

As I see it, 2) would defeat a bit the purpose of reusable components. Basically all the Yoast components should slightly differentiate some small design details when used on WP or My Yoast. Not sure this is the direction we want to take but can certainly be done if really necessary.

hedgefield · 2017-10-17T08:58:11Z

I agree, keeping it reusable across all products is best. I just want to be careful that we don't deviate too much from the wordpress look, intense drop shadows don't really fit there. The help center one is too much. Use the yoast alert one for now. I'll make a separate issue to fix the shadows across all the things, in wp and in my yoast.

I agree nesting papers is not good, this will get better in time. 🙃

afercia · 2017-10-17T12:32:24Z

Latest changes implement the 24/16 pixels padding and change the box-shadow as requested:

Use the yoast alert one

IreneStr · 2017-10-19T06:52:08Z

CR done 👍
I've removed an unnecessary semicolon.

IreneStr · 2017-10-19T08:04:29Z

Acceptance done 👍

Added Components Notification component. #348 AnalysisResult component. #352 ContentAnalysis component. #359 Other additions Added callbacks to notify tab switch in YoastTabs. #346 setSeoResultsForKeyword action and reducer implementation. #355 Added default className to VideoDescriptionItem component. #358

afercia added 2 commits October 13, 2017 17:28

Add Notification component.

00bac0f

Add translatable aria-label.

a62d823

Set the content as HTML.

c6f1f3e

IreneStr reviewed Oct 16, 2017

View reviewed changes

afercia added 2 commits October 16, 2017 12:28

Responsive.

3467170

Set the heading right margin only when the close button is displayed.

434720b

afercia added 2 commits October 17, 2017 14:23

Adjust padding and shadow to process feedback.

d421fd2

Merge branch 'develop' into stories/347-notification-component

e78b410

Remove unnecessary semicolon

e79e94e

Update tests

e91ca1d

IreneStr merged commit abd8d9c into develop Oct 19, 2017

IreneStr added this to the 2.11 milestone Oct 19, 2017

moorscode deleted the stories/347-notification-component branch October 31, 2017 08:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add notification component #348

Add notification component #348

afercia commented Oct 13, 2017 •

edited

Loading

IreneStr commented Oct 16, 2017

afercia commented Oct 16, 2017

hedgefield commented Oct 16, 2017

IreneStr commented Oct 16, 2017

hedgefield commented Oct 16, 2017

IreneStr Oct 16, 2017

afercia Oct 16, 2017

IreneStr Oct 16, 2017

hedgefield Oct 17, 2017

IreneStr Oct 16, 2017

IreneStr Oct 16, 2017

afercia Oct 16, 2017 •

edited

Loading

IreneStr Oct 16, 2017

IreneStr Oct 16, 2017

afercia Oct 16, 2017

afercia Oct 16, 2017

hedgefield Oct 17, 2017

IreneStr Oct 16, 2017

IreneStr Oct 16, 2017

afercia Oct 16, 2017

IreneStr Oct 16, 2017

hedgefield Oct 17, 2017

afercia commented Oct 16, 2017

afercia commented Oct 17, 2017

hedgefield commented Oct 17, 2017

afercia commented Oct 17, 2017

IreneStr commented Oct 19, 2017 •

edited

Loading

IreneStr commented Oct 19, 2017

		@@ -0,0 +1,3 @@
		<svg width="1792" height="1792" viewBox="0 0 1792 1792" xmlns="http://www.w3.org/2000/svg">

Add notification component #348

Add notification component #348

Conversation

afercia commented Oct 13, 2017 • edited Loading

Summary

Relevant technical choices:

Test instructions

IreneStr commented Oct 16, 2017

afercia commented Oct 16, 2017

hedgefield commented Oct 16, 2017

IreneStr commented Oct 16, 2017

hedgefield commented Oct 16, 2017

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

afercia Oct 16, 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

afercia commented Oct 16, 2017

afercia commented Oct 17, 2017

hedgefield commented Oct 17, 2017

afercia commented Oct 17, 2017

IreneStr commented Oct 19, 2017 • edited Loading

IreneStr commented Oct 19, 2017

afercia commented Oct 13, 2017 •

edited

Loading

afercia Oct 16, 2017 •

edited

Loading

IreneStr commented Oct 19, 2017 •

edited

Loading