Add a dismissable Alert component with 4 different styles: info, warning, error & success. #332

Dieterrr · 2019-08-05T14:00:06Z

Summary

Adds an Alert component to Yoast components. It has 4 different styles that can be set in its props: info, warning, error & success. It has an option to be dismissable, which will be saved in a cookie. The alert is based on this design: https://github.com/Yoast/design/issues/371#issuecomment-487953202

Package(s) involved:
Should the change be included in the package changelog?
- No
- Yes
Should the change be included in one or more plugin changelogs?
- No
- Free
- Premium
- Other (please specify)
Package changelog item (if applicable):
Adds a dismissable Alert component with 4 different styles: info, warning, error & success.

Relevant technical choices:

Added the component to the example App under the section 'components'.
Removed the svg files and replaced them with inline generated svg files in the alert component.
Wrapped the App in to make the component work, as it uses react-intl.
Added grunt to @yoast/style-guide to be able to build the colors.json file.
Added all the required colors to _colors.scss. It's a lot of new colors so it makes the file a bit messy. Would like a second opinion on how to make this cleaner.

Test instructions

This PR can be tested by following these steps:

Checkout this branch. Go into the apps/components folder and run yarn install, then yarn start
Open the website on the provided port and go into the components tab.
Here you should see the following 4 alerts:

* Inspect them and make sure they match the specifications here: https://github.com/Yoast/design/issues/371#issuecomment-487953202 * Close them and make sure they don't come back unless you delete the cookies :) * If you want to be thorough you can also yarn link to myYoast and test the alerts there.

Impact check

UI changes

This PR changes the UI in the plugin. I have added the 'UI change' label to this PR.

Quality assurance

I have tested this code to the best of my abilities
I have added unittests to verify the code works as intended

Fixes #331

afercia · 2019-08-06T12:05:18Z

packages/components/src/Alert.js

+	 * @returns {void}
+	 */
+	onCrossClick() {
+		Cookies.set( this.props.cookieName, "true", { expires: 7 } );


Though the code checks only for the existence of the cookie (so that the actual cookie value doesn't matter) I think the cookie value is slightly confusing. It confused me also when CR'ing https://github.com/Yoast/my-yoast/pull/2790

For example. when an alert is hidden the actual cookie set could be something like:
errorAlert = "true"

At a first glance, I'd read that as "yes, display the error alert", while it means "hide it" instead. I'd just suggest to change the cookie value to hide.

afercia · 2019-08-06T14:12:19Z

packages/components/src/Alert.js

+	cookieName: "",
+};
+
+export default injectIntl( Alert );


When the Alert was used only in MyYoast, injectIntl made sense. Now that is a package component, not sure this is what we'd want to use.

For example, when importing and using the Alert in Yoast SEO there would be the need to wrap the component in an IntlProvider component. Instead, on Yoast SEO we're using @wordpress/i18n. I think this should be changed to use __() but I'd like confirmation from the architects. If so, all the code related to react-intl (including in App.js) should be removed in favor of @wordpress/i18n.

For reference, this is the Alert imported and used in Yoast SEO:

afercia · 2019-08-06T14:24:08Z

CR 👍
Acceptance 🚧

1
Important: I think the method used for translations should be changed in favor of @wordpress/i18n. Please see comment in the CR.

2
When used in Yoast SEO, the "close" button won't have any focus style when using Firefox. This is because the button inherits CSS rules from WordPress that remove the proprietary styles for ::-moz-focus-inner. We should provide a custom focus style and not rely on the browsers native focus style. See https://github.com/Yoast/bugreports/issues/511

3
There are no tests. Not sure if this was agreed internally. Maybe add a basic snapshot test?

4
The "close" button size is 20 by 20 pixels. I'd say that on mobile it's too small. We should consider to make it bigger on small screens. This would require design feedback and I guess it can be split in a separate issue.

5
Note: importing and using the Alert on Yoast SEO was a painful experience for me. Maybe I'm missing some important detail but between linking the monorepo, yarn install & Co. I was getting an error Module not found: Error: Can't resolve 'js-cookie' in '/Users/andrea/wp/plugins/wordpress-seo/node_modules/@yoast/components/src'. No idea why, as it works in the standalone example. I had to manually yarn add js-cookie to Yoast SEO. I'd suggest to double check this point.

The Alert tested on a Yoast SEO page:

Dieterrr · 2019-08-08T14:53:32Z

@afercia Thanks for your extensive review. I have implemented the changes as following:

You're right. I had to gain some context on the translation dependencies. I have replaced react-intl with @wordpress/i18n.
I let this how it is and assume the bugreport you filed will fix it?
I added four snapshot tests.
I think this should be a separate issue as it would indeed involve a design decision.
I checked with Herre and this should not be an issue. The 'js-cookie' package is added to @yoast/components in this PR. Are all the files build correctly?

afercia · 2019-08-09T08:37:47Z

Thanks @Dieterrr !

4
Will create a new issue

5
Maybe I'm missing something. Just tested again. What I do is:

switch the javascript repo to this branch and run yarn on the project root
on wordpress-seo trunk, run yarn link-monorepo
run yarn && grunt build:css && grunt build:js && grunt webpack:buildProd
error:

    ERROR in ./node_modules/@yoast/components/src/Alert.js
    Module not found: Error: Can't resolve 'js-cookie' in '/Users/andrea/wp/plugins/wordpress-seo/node_modules/@yoast/components/src'
     @ ./node_modules/@yoast/components/src/Alert.js 22:16-36
     @ ./node_modules/@yoast/components/src/index.js
     @ ./js/src/components.js

The only way to fix it for me is by adding js-cookie manually: yarn add js-cookie. I also tried deleting all the node_modules directories in all projects, no luck.

afercia · 2019-08-09T10:10:12Z

Even after I manually add js-cookie and try to import and use the Alert in the Yoast SEO plugin (see attached diff), I don't get the colors. this.options.color and this.options.background. The colors.json file doesn't contain these new colors. Is this an issue related to versions>

courses-overview.diff.txt

JoseeWouters · 2019-08-12T12:05:37Z

I changed the typo for the link color.

Acceptance 🚧
My local environment fails when I try to test in MyYoast. Could you look at this and make sure it's not because of the PR?

Dieterrr · 2019-08-12T16:29:06Z

I merged with develop and fixed the snapshots.

I'm currently getting the following error in myYoast when I link it.

styled-components.browser.cjs.js:2479 It looks like there are several instances of 'styled-components' initialized in this application. This may cause dynamic styles not rendering properly, errors happening during rehydration process and makes your application bigger without a good reason.

See https://s-c.sh/2BAXzed for more info.
(anonymous) @ styled-components.browser.cjs.js:2479
(anonymous) @ styled-components.browser.cjs.js:2503
__webpack_require__ @ bootstrap 672f71022272ce7e07ff:582
fn @ bootstrap 672f71022272ce7e07ff:106
(anonymous) @ Button.js:2
__webpack_require__ @ bootstrap 672f71022272ce7e07ff:582
fn @ bootstrap 672f71022272ce7e07ff:106
(anonymous) @ Loader.js:12
__webpack_require__ @ bootstrap 672f71022272ce7e07ff:582
fn @ bootstrap 672f71022272ce7e07ff:106
(anonymous) @ Loader.js:2
__webpack_require__ @ bootstrap 672f71022272ce7e07ff:582
fn @ bootstrap 672f71022272ce7e07ff:106
(anonymous) @ SitesPage.js:11
__webpack_require__ @ bootstrap 672f71022272ce7e07ff:582
fn @ bootstrap 672f71022272ce7e07ff:106
(anonymous) @ SitesPage.js:5
__webpack_require__ @ bootstrap 672f71022272ce7e07ff:582
fn @ bootstrap 672f71022272ce7e07ff:106
(anonymous) @ Menu.js:11
__webpack_require__ @ bootstrap 672f71022272ce7e07ff:582
fn @ bootstrap 672f71022272ce7e07ff:106
(anonymous) @ App.js:20
__webpack_require__ @ bootstrap 672f71022272ce7e07ff:582
fn @ bootstrap 672f71022272ce7e07ff:106
(anonymous) @ index.js:3
(anonymous) @ index.js:87
__webpack_require__ @ bootstrap 672f71022272ce7e07ff:582
fn @ bootstrap 672f71022272ce7e07ff:106
(anonymous) @ bootstrap 672f71022272ce7e07ff:628
__webpack_require__ @ bootstrap 672f71022272ce7e07ff:582
(anonymous) @ bootstrap 672f71022272ce7e07ff:628
(anonymous) @ bootstrap 672f71022272ce7e07ff:628
Show 3 more frames
styled-components.browser.cjs.js:213 Uncaught Error: Trying to insert a new style tag, but the given Node is unmounted!

- Are you using a custom target that isn't mounted?
- Does your document not have a valid head element?
- Have you accidentally removed a style tag manually?

@afercia I have reproduced the issue with the colors. When I log it instead of a color string it is undefined. I wonder if it's related to linking it because if I open the json file I the colors are there. I will look into it and into the js-cookie problem.

This reverts commit bfcc585.

Dieterrr · 2019-08-20T11:54:15Z

I have put this in blocked and created an issue as the issues with webpack and linking the javascript monorepo were too time consuming and outside of the scope of this PR.

Here is the issue:
https://github.com/Yoast/my-yoast/issues/2840

I don't think the current linking between MyYoast and the javascript monorepo is in a usable state and would propose not using it until these issues are fixed.

afercia · 2019-09-09T15:19:08Z

Noting there's now a new PR #349 and maybe this one should be closed.

Dieter and others added 5 commits August 2, 2019 15:43

Added Alert component and required icons

b5592bc

Added Alert component to components, added example to App

4cdadce

Fixed eslint errors

b84c7b8

CSS and docs improvements.

aad4bcb

Commit modified yarn lock file.

b2cef0b

afercia reviewed Aug 6, 2019

View reviewed changes

Change the cookie value.

2d874e1

afercia reviewed Aug 6, 2019

View reviewed changes

Dieter added 5 commits August 8, 2019 11:31

Replaced react-intl with wordpress-i18n, changed cookie value for hide

f1c47fe

added yarn.lock

305dcb9

merged origin

60ee505

Added 4 snapshot tests

eb06ca5

CS update

f436616

Dieterrr force-pushed the add-alert-component branch from 0e53dc3 to f436616 Compare August 8, 2019 14:37

Dieter added 2 commits August 8, 2019 17:11

updated snapshot tests for search-metadata

103f071

updated warnings

92761fc

danielFangstrom approved these changes Aug 9, 2019

View reviewed changes

fix color for links

9248de8

Dieter added 2 commits August 12, 2019 18:12

Merge branch 'develop' into add-alert-component

f2e0afa

merged develop, updated tests

37bd9d3

Dieter added 4 commits August 19, 2019 17:17

reverted yarn.lock

bfcc585

Merge branch 'develop' into add-alert-component

d2083a2

add new yarn.lock file

a0001e5

Revert "reverted yarn.lock"

c398929

This reverts commit bfcc585.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a dismissable Alert component with 4 different styles: info, warning, error & success. #332

Add a dismissable Alert component with 4 different styles: info, warning, error & success. #332

Dieterrr commented Aug 5, 2019

afercia Aug 6, 2019

afercia Aug 6, 2019

afercia commented Aug 6, 2019

Dieterrr commented Aug 8, 2019 •

edited

Loading

afercia commented Aug 9, 2019

afercia commented Aug 9, 2019

JoseeWouters commented Aug 12, 2019

Dieterrr commented Aug 12, 2019 •

edited

Loading

Dieterrr commented Aug 20, 2019

afercia commented Sep 9, 2019

Add a dismissable Alert component with 4 different styles: info, warning, error & success. #332

Are you sure you want to change the base?

Add a dismissable Alert component with 4 different styles: info, warning, error & success. #332

Conversation

Dieterrr commented Aug 5, 2019

Summary

Relevant technical choices:

Test instructions

Impact check

UI changes

Quality assurance

afercia Aug 6, 2019

Choose a reason for hiding this comment

afercia Aug 6, 2019

Choose a reason for hiding this comment

afercia commented Aug 6, 2019

Dieterrr commented Aug 8, 2019 • edited Loading

afercia commented Aug 9, 2019

afercia commented Aug 9, 2019

JoseeWouters commented Aug 12, 2019

Dieterrr commented Aug 12, 2019 • edited Loading

Dieterrr commented Aug 20, 2019

afercia commented Sep 9, 2019

Dieterrr commented Aug 8, 2019 •

edited

Loading

Dieterrr commented Aug 12, 2019 •

edited

Loading