Implement help text in react snippet editor #517

danielFangstrom · 2018-05-09T10:05:08Z

Summary

This PR can be summarized in the following changelog entry:

Adds a button to the react snippet editor that toggles the help text

Relevant technical choices:

Uses a wrapper that contains the button and the div
The HelpText component is included from the feature/content-analysis-in-sidebar branch and has already been code reviewed

Test instructions

This PR can be tested by following these steps:

Use define( 'YOAST_FEATURE_SNIPPET_PREVIEW', true ); in wordpress-seo to see the react snippet editor (for example on line 46 in class-metabox-formatter.php)
There should be a question mark button in the react snippet preview that should toggle help text above the snippet title
The appearance of the button and the text should be the same as in the current snippet preview (including focus and hover styles)
Running yarn test should pass all tests

Fixes #481

maartenleenders

CR done 🚧

maartenleenders · 2018-05-09T12:28:26Z

composites/Plugin/SnippetPreview/components/HelpTextWrapper.js

+
+
+const HelpTextContainer = styled.div`
+ max-width: 50em;


indentation?

maartenleenders · 2018-05-09T12:29:02Z

composites/Plugin/SnippetPreview/components/HelpTextWrapper.js

+`;
+
+let HelpTextDiv = styled.div`
+ max-width: 30em !important;


Was this !important really necessary or is it a remnant from experimenting?

Why is it in ems instead of px/%?

This was copied from the specifications of the old snippet editor help text, but I have adjusted it to use pixels now instead.

maartenleenders · 2018-05-09T12:29:59Z

composites/Plugin/SnippetPreview/components/HelpTextWrapper.js

+ max-height: 0;
+`;
+
+const colorHelpTextButton = "#0073aa";


Isn't this color in the colors.json? Or, if not, a color that's very close?

And please move this to above the styled components

maartenleenders · 2018-05-09T12:31:36Z

composites/Plugin/SnippetPreview/components/HelpTextWrapper.js

+
+const colorHelpTextButton = "#0073aa";
+
+const HelpTextContent = [ "This is a rendering of what this post might look like in Google's search results. ",


Please move this to above the styled components

maartenleenders · 2018-05-09T12:32:51Z

composites/Plugin/SnippetPreview/components/HelpTextWrapper.js

+ /**
+ * Renders the help text wrapper.
+ *
+ * @returns {ReactElement} The rendered help text wrapper .


space before period

maartenleenders · 2018-05-09T12:41:11Z

composites/Plugin/SnippetPreview/components/HelpTextWrapper.js

+ render() {
+ return (
+ <HelpTextContainer
+ className={ `${ this.props.className }` }>


Plkease try without ${

maartenleenders · 2018-05-09T12:41:58Z

composites/Plugin/SnippetPreview/components/HelpTextWrapper.js

+ >
+ <StyledSvg
+ size="16px"
+ color="#72777c"


Please use a color from colors.json (this one or a close one)

maartenleenders · 2018-05-09T12:48:30Z

composites/Plugin/SnippetPreview/components/SnippetPreview.js

@@ -615,6 +566,9 @@ export default class SnippetPreview extends PureComponent {
 /* eslint-disable jsx-a11y/mouse-events-have-key-events */
 return (
 <section>
+ <div>
+ <HelpTextWrapper />


Please pass the specific help text as a prop here, instead of hardcoding it in the wrapper. This way, the wrapper remains reusable.

danielFangstrom · 2018-05-11T07:52:03Z

Implemented most changes from the CR but temporarily on hold until a decision is made on the colors.

…le from the develop branch.

…ies/implement-help-text-in-snippet-preview-editor

boblinthorst · 2018-05-15T09:25:07Z

composites/Plugin/SnippetPreview/components/HelpTextWrapper.js

+ margin: 0px 20px 10px 25px
+`;
+
+let HelpTextDiv = styled.div`


Looks like it could be a const instead of a let?

I change the properties of the div depending on a conditional in the render method, so I thought I needed let but it turns out that const works as well. Changed to the latter.

boblinthorst · 2018-05-15T09:25:17Z

composites/Plugin/SnippetPreview/components/HelpTextWrapper.js

+
+
+const HelpTextContainer = styled.div`
+ max-width: 600px;


One tab too much?

boblinthorst · 2018-05-15T09:25:54Z

composites/Plugin/SnippetPreview/components/HelpTextWrapper.js

+ max-width: 600px;
+ padding: 1 0 3 1em;
+ font-weight: normal;
+ margin: 0px 20px 10px 25px


If 0px, replace with 0.

boblinthorst · 2018-05-15T09:27:38Z

composites/Plugin/SnippetPreview/components/HelpTextWrapper.js

+ box-shadow: 0 0 0 0px #5b9dd9;
+ position: relative;
+ display: block;
+ margin: -44px -10px 10px 0px;


boblinthorst · 2018-05-15T10:19:31Z

composites/Plugin/SnippetPreview/components/HelpTextWrapper.js

+ border-radius: 50%;
+ border: none;
+ clip: rect(1px 1px 1px 1px);
+ box-shadow: 0 0 0 0px #5b9dd9;


0px to 0, is there a color in colors.json that corresponds to the box shadow color?

boblinthorst · 2018-05-15T10:20:12Z

composites/Plugin/SnippetPreview/components/HelpTextWrapper.js

+ margin: -44px -10px 10px 0px;
+ background-color: transparent;
+ float: right;
+ padding: 2px 0px 2px 6px;


boblinthorst · 2018-05-15T10:23:38Z

composites/Plugin/SnippetPreview/components/HelpTextWrapper.js

+ /**
+ * Handles the onButtonClick event.
+ *
+ * Toggles the boolean isExpanded in the state


boblinthorst · 2018-05-15T10:31:30Z

composites/Plugin/SnippetPreview/components/HelpTextWrapper.js

+ * Renders the HelpTextWrapper component
+ *
+ * @param {Object} props The passed props.
+ * @param {string} props.className The class name.


Either line up the documentation, or keep the seperation space between the description and the variable name the same, now it's inconsistent. And should the helpText prop also be documented here?

boblinthorst · 2018-05-15T11:09:21Z

composites/Plugin/SnippetPreview/components/SnippetPreview.js

@@ -659,6 +663,9 @@ export default class SnippetPreview extends PureComponent {
 /* eslint-disable jsx-a11y/mouse-events-have-key-events */
 return (
 <section>
+ <div>
+ <HelpTextWrapper helpText = { helpText } />


no spacing around the = sign.

boblinthorst · 2018-05-15T11:10:41Z

composites/Plugin/SnippetPreview/components/SnippetPreview.js

@@ -53,6 +54,9 @@ const angleRight = ( color ) => "data:image/svg+xml;charset=utf8," + encodeURI(
 "</svg>"
 );

+export const helpText = [ "This is a rendering of what this post might look like in Google's search results. ",


looks like it should be translatable

boblinthorst · 2018-05-15T11:12:46Z

Cr: placed some comments

abotteram · 2018-05-15T13:12:21Z

CR 👍

…e helptext.

igorschoester · 2018-05-16T07:21:10Z

Acceptance 🚧

In Firefox it has a shadow effect:

On Safari is has a slight shadow effect when pressing the button. This style is always applied, just visible in Safari:

.arVCw:active {
    box-shadow: inset 0 2px 5px -3px rgba( 0,0,0,0.5 );
}

igorschoester · 2018-05-16T09:30:40Z

Acceptance 👍

danielFangstrom added this to the 3.6 milestone May 9, 2018

maartenleenders reviewed May 9, 2018

View reviewed changes

danielFangstrom added 3 commits May 11, 2018 14:28

Fix the branching issue which caused reversal of code added to the fi…

ca22b6b

…le from the develop branch.

Merge branch 'develop' of github.com:Yoast/yoast-components into stor…

43df259

…ies/implement-help-text-in-snippet-preview-editor

Resolve merge conflict.

bc48778

danielFangstrom force-pushed the stories/implement-help-text-in-snippet-preview-editor branch from ac1fd00 to bc48778 Compare May 14, 2018 13:11

Add updated snapshot, and fix accidental change of unrelated file.

10e3d95

boblinthorst reviewed May 15, 2018

View reviewed changes

danielFangstrom and others added 7 commits May 15, 2018 14:01

Implement changes requested in CR.

8ae9b85

Merge develop into branch.

8e849fd

Fix text appearance.

938c708

Change styled div to const and update snapshots.

eb9a33b

Remove space around equality sign in render method.

ac3db74

Removed superfluous padding from HelpTextContainer

69db7ee

Fixed documentation

baacf7e

Fix alignment of button and change the textcolor and font style of th…

9d3a14b

…e helptext.

Fix box-shadow issues

fd79760

Update tests

bf32d46

igorschoester merged commit 686732b into develop May 16, 2018

igorschoester deleted the stories/implement-help-text-in-snippet-preview-editor branch May 16, 2018 09:30

afercia mentioned this pull request May 17, 2018

Fix accessibility of the snippet editor help text #541

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement help text in react snippet editor #517

Implement help text in react snippet editor #517

danielFangstrom commented May 9, 2018 •

edited

Loading

maartenleenders left a comment

maartenleenders May 9, 2018

maartenleenders May 9, 2018

maartenleenders May 9, 2018 •

edited

Loading

danielFangstrom May 9, 2018

maartenleenders May 9, 2018

maartenleenders May 9, 2018

maartenleenders May 9, 2018

maartenleenders May 9, 2018

maartenleenders May 9, 2018

maartenleenders May 9, 2018

maartenleenders May 9, 2018

danielFangstrom commented May 11, 2018

boblinthorst May 15, 2018

danielFangstrom May 15, 2018

boblinthorst May 15, 2018

boblinthorst May 15, 2018

boblinthorst May 15, 2018

boblinthorst May 15, 2018

boblinthorst May 15, 2018

boblinthorst May 15, 2018

boblinthorst May 15, 2018

boblinthorst May 15, 2018

boblinthorst May 15, 2018

boblinthorst commented May 15, 2018

abotteram commented May 15, 2018

igorschoester commented May 16, 2018 •

edited

Loading

igorschoester commented May 16, 2018


		const colorHelpTextButton = "#0073aa";

		const HelpTextContent = [ "This is a rendering of what this post might look like in Google's search results. ",

Implement help text in react snippet editor #517

Implement help text in react snippet editor #517

Conversation

danielFangstrom commented May 9, 2018 • edited Loading

Summary

Relevant technical choices:

Test instructions

maartenleenders left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maartenleenders May 9, 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

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

danielFangstrom commented May 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

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

boblinthorst commented May 15, 2018

abotteram commented May 15, 2018

igorschoester commented May 16, 2018 • edited Loading

igorschoester commented May 16, 2018

danielFangstrom commented May 9, 2018 •

edited

Loading

maartenleenders May 9, 2018 •

edited

Loading

igorschoester commented May 16, 2018 •

edited

Loading