Add styles for Notice component #8856

mmtr · 2018-08-10T22:35:15Z

It adds the needed styles by the Notice component, so it is displayed correctly even if the WordPress Core styles are not available.

mmtr · 2018-08-11T11:06:24Z

@gziolo @youknowriad

youknowriad · 2018-08-13T07:41:08Z

packages/components/src/notice/index.js

 import { __ } from '@wordpress/i18n';

 function Notice( { className, status, children, onRemove = noop, isDismissible = true } ) {
-	const classNames = classnames( className, 'notice notice-alt notice-' + status, {
+	const classNames = classnames( className, 'components-notice components-notice--' + status, {


Should this be __ instead of --

Actually, it's looks like this is a modifier. I think in general our guideline is to use unprefixed booleans. So maybe just: is-warning, is-error...

@jasmussen will know what's the best way to deal with it 😃

I think I'm feeling what Riad is saying.

I believe we have these notices:

Green (Success)

Orange (Warning)

Red (Error)

I can't recall if there's also a blue informational notice, if there isn't there probably should be.

The default notice should be an informational notice. In addition to that, we could have is-succes, is-warning, is-error.

Did this make sense?

Yep. It's consistent with what we do in other components. I have just committed the change using these new modifiers.

youknowriad · 2018-08-15T13:16:53Z

The dismiss button doesn't look centered vertically to me. Is this related to this changes?

youknowriad · 2018-08-15T14:22:02Z

packages/editor/src/components/contrast-checker/index.js

@@ -36,7 +36,7 @@ function ContrastChecker( {
 		__( 'This color combination may be hard for people to read. Try using a brighter background color and/or a darker text color.' );
 	return (
 		<div className="editor-contrast-checker">
-			<Notice status="warning" isDismissible={ false }>
+			<Notice status="warning" isDismissible={ true }>


debug change ;)

mmtr · 2018-08-15T14:39:06Z

I've just realized that the original WordPress notice was placing the dismiss button at the top right corner, so I have removed the vertical alignment.

Original:

New:

youknowriad · 2018-08-15T15:06:20Z

packages/components/src/notice/index.js

@@ -7,17 +7,21 @@ import classnames from 'classnames';
 /**
 * WordPress dependencies
 */
+import Dashicon from '../dashicon';


This should be moved to "Internal dependencies"

youknowriad · 2018-08-15T15:07:28Z

packages/components/src/notice/index.js

 			{ isDismissible && (
-				<button className="notice-dismiss" type="button" onClick={ onRemove }>
+				<button className="components-notice__dismiss" type="button" onClick={ onRemove }>


Can we replace this by IconButton instead of button. I think it already takes care of the scree reader label (label prop) etc...

I tried to do it, but it presents a problem with the hover styles. Either we use exactly the same rule defined in the IconButton styles for the Notice styles so we have the same specificness, or we add a !important to override it.

I didn't like any of the options, so that's why I left the button. What do you think?

cc @jasmussen for design insights but I'm pretty sure we can override without !important (repeating the same specificity, granted that it's not great either)

Btw, I don't consider this a blocker for the PR.

Yeah, those hover styles are a bit arduous due to having to work with a number of accessibility concerns. However you can override them without !importants, by using the same level of specificity.

For example, if the hover is .components-icon-button:not(:disabled):_not([aria-disabled="true"]):not(.is-default):hover, you can override it with .components-notice__dismiss.components-icon-button:not(:disabled):_not([aria-disabled="true"]):not(.is-default):hover.

But would also not call that a blocker.

Yep. My only concern is that this is less maintainable. If we change the style rules for IconButton component, we might need to also change them in the Notice component.

Anyway, I have overridden them with:

&:not(:disabled):not([aria-disabled="true"]):not(.is-default):hover, &:not(:disabled):not([aria-disabled="true"]):not(.is-default):active, &:not(:disabled):not([aria-disabled="true"]):focus { color: $alert-red; background-color: transparent; } &:not(:disabled):not([aria-disabled="true"]):not(.is-default):hover { box-shadow: none; }

Cool — do you really need to override the color though? $alert-red as a hover/active color doesn't seem necessary, and it probably will run into contrast issues. Probably just keep the color as is?

youknowriad · 2018-08-16T09:06:33Z

edit-post/components/layout/style.scss

 			margin: 0 0 5px;
 			padding: 6px 12px;
 			min-height: $panel-header-height;

-			.notice-dismiss {
+			.components-notice__dismiss {
 				margin: 5px;


Can we tweak this margin to make it appear centered by default if the message is in a single line ?

margin: 10px 5px;

youknowriad

LGTM 👍

youknowriad · 2018-08-16T14:47:02Z

I went ahead and tweaked the margin a bit. Let me know if it doesn't work for you.

afercia · 2018-08-30T09:46:40Z

This PR made the dismiss button tooltip not visible with tooltip={ false }. Worth reminding all UI controls must have a visible label, or at least a way to expose their accessible name (and tooltips are already a trade-off).
Imagine speech recognition software user: what command are they supposed to voice to dismiss the notices?
"Click X"? won't work
"Click close"? won't work
How are they supposed to discover the control name is "Dismiss this notice" if it's not exposed in the UI in any way? The tooltip allows, at least, to discover what the control name is and once users have learned it, they can use it directly the following times just voicing the command "Click Dismiss this button".

Will fix in #9443.

mmtr mentioned this pull request Aug 11, 2018

DevDocs: Gutenberg components - Updating examples Automattic/wp-calypso#26367

Merged

youknowriad reviewed Aug 13, 2018

View reviewed changes

gziolo requested a review from jasmussen August 13, 2018 07:52

gziolo added the [Package] Components /packages/components label Aug 13, 2018

gziolo added this to the 3.6 milestone Aug 14, 2018

gziolo added the [Type] Enhancement A suggestion for improvement. label Aug 14, 2018

mmtr added 8 commits August 14, 2018 21:22

Add styles for Notice component

f04fca3

Add styles for Notice component

292427d

Add styles for Notice component

3e50d0b

Fixing stylelint issues and updating snapshots

a2d6d9c

Fixing e2e tests

a527034

Fixing e2e tests

d1e7f42

Rename notice modifier classes

a2e3711

Rename notice modifier classes

750d4f7

mmtr added 2 commits August 15, 2018 15:56

Notice components: Align vertically the dismiss button

ce043c0

Notice components: Align vertically the dismiss button

815636b

youknowriad reviewed Aug 15, 2018

View reviewed changes

Notice components: Placing dismiss button at the top right corner

8e2b58e

Notice component: Fixing styles for new spinner components

b158914

youknowriad reviewed Aug 15, 2018

View reviewed changes

mmtr added 2 commits August 15, 2018 17:43

Notice component: Using IconButton as dismiss button

86ef46e

Notice component: Updating snapshot

7689ec4

youknowriad reviewed Aug 16, 2018

View reviewed changes

youknowriad approved these changes Aug 16, 2018

View reviewed changes

Center the dismiss button

f3495b1

youknowriad merged commit 837d908 into WordPress:master Aug 16, 2018

youknowriad mentioned this pull request Sep 10, 2018

Fix notice styling regression #9736

Merged

talldan mentioned this pull request Aug 4, 2021

Info/Information Notice Styling is absent #33853

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add styles for Notice component #8856

Add styles for Notice component #8856

mmtr commented Aug 10, 2018

mmtr commented Aug 11, 2018

youknowriad Aug 13, 2018

youknowriad Aug 13, 2018

gziolo Aug 13, 2018

jasmussen Aug 13, 2018

mmtr Aug 13, 2018

youknowriad commented Aug 15, 2018

youknowriad Aug 15, 2018

mmtr Aug 15, 2018

mmtr commented Aug 15, 2018

youknowriad Aug 15, 2018

youknowriad Aug 15, 2018

mmtr Aug 15, 2018 •

edited

Loading

youknowriad Aug 15, 2018

youknowriad Aug 15, 2018

jasmussen Aug 16, 2018

mmtr Aug 16, 2018 •

edited

Loading

jasmussen Aug 16, 2018

youknowriad Aug 16, 2018

youknowriad left a comment

youknowriad commented Aug 16, 2018 •

edited

Loading

afercia commented Aug 30, 2018 •

edited

Loading

Add styles for Notice component #8856

Add styles for Notice component #8856

Conversation

mmtr commented Aug 10, 2018

mmtr commented Aug 11, 2018

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

youknowriad commented Aug 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmtr commented Aug 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmtr Aug 15, 2018 • 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

mmtr Aug 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad left a comment

Choose a reason for hiding this comment

youknowriad commented Aug 16, 2018 • edited Loading

afercia commented Aug 30, 2018 • edited Loading

mmtr Aug 15, 2018 •

edited

Loading

mmtr Aug 16, 2018 •

edited

Loading

youknowriad commented Aug 16, 2018 •

edited

Loading

afercia commented Aug 30, 2018 •

edited

Loading