10267 add cornerstone toggle to sidebar #668

Dieterrr · 2018-07-19T13:02:25Z

Summary

This PR can be summarized in the following changelog entry:

Adds a toggle to the Yoast Sidebar. The toggle is styled the same as the Gutenberg toggle.

Relevant technical choices:

As WP components is not (yet) available, it was not possible to import the toggle component from WP and we had to copy the code into our own file named GutenbergToggle.js. When the WP component becomes available we can remove this code and simply import the component.
We did not include the SCSS file from WP in our code, as it is already loaded in WordPress when Gutenberg is enabled. This means the button works in the plugin sidebar, but does not display correctly in the standalone app.
The toggle button currently "toggles", for testing purposes. In the future this functionality needs to use the yoast_wpseo_is_cornerstone value. For this we need the the finished container which houses the cornerstone toggle and is connected to the store.

Test instructions

This PR can be tested by following these steps:

Checkout out the branches 10267-add-cornerstone-toggle-to-sidebar in both components as well as Wordpress SEO and yarn link them (not needed when this is an RC).
Make sure define( 'YOAST_FEATURE_GUTENBERG_SIDEBAR', true ); is set in wp-config.php.
Edit a post or page, look at the Yoast sidebar (under the plug icon) and see the toggle :). The toggle should switch on/off when clicked and it's styling should be the same as the Gutenberg toggles found under the settings/gear icon.

Fixes #10267
Requires #10405

afercia · 2018-07-19T14:17:45Z

app/KeywordExample.js

@@ -5,6 +5,7 @@ import styled from "styled-components";
 // Internal dependencies.
 import KeywordInput from "../composites/Plugin/Shared/components/KeywordInput";
 import SynonymsSection from "../composites/Plugin/Synonyms/components/SynonymsSection";
+import CornerstoneToggle from "../composites/Plugin/CornerstoneContent/CornerstoneToggle";


this path should be "../composites/Plugin/CornerstoneContent/components/CornerstoneToggle"

afercia · 2018-07-19T14:19:51Z

composites/Plugin/CornerstoneContent/components/CornerstoneToggle.js

+        margin-right: 10px;
+        flex-shrink: 0;
+        max-width: 75%;
+    }


the indentation in all these lines is made of spaces. Should be tabs.

afercia · 2018-07-19T14:20:54Z

composites/Plugin/CornerstoneContent/components/CornerstoneToggle.js

@@ -0,0 +1,81 @@
+import React from "react";
+import PropTypes from "prop-types";
+import Styled from "styled-components";


Styled should be lowercase

afercia · 2018-07-19T14:21:05Z

composites/Plugin/CornerstoneContent/components/CornerstoneToggle.js

+
+import GutenbergToggle from "../../Shared/components/GutenbergToggle";
+
+const Cornerstone = Styled.div`


Styled should be lowercase

afercia · 2018-07-19T14:26:55Z

composites/Plugin/CornerstoneContent/components/CornerstoneToggle.js

+					key={this.props.key}
+					checked={this.state.cornerstoneToggleState}
+					onChange={this.handleChange}
+					id={this.props.toggleId}


coding standards: all these lines (57 and 59-62) miss spaces between brackets.

The label should be always correctly associated with the input ID, more details in the CR comment.

afercia · 2018-07-19T14:29:16Z

composites/Plugin/Shared/components/GutenbergToggle.js

+ * External dependencies
+ */
+import React from "react";
+import classnames from "classnames";


has the introduction of the classnames package been discussed with @atimmer ?

afercia · 2018-07-19T14:32:38Z

composites/Plugin/Shared/components/GutenbergToggle.js

+		className,
+		{ 'is-checked': checked },
+	);
+


so to my understanding, this gets properly styled only when in Gutenberg. When used outside Gutenberg, it will be completely unstyled (as in the standalone app example > "Keyword" tab). Is this what we want?

afercia · 2018-07-19T14:34:28Z

composites/Plugin/Shared/components/GutenbergToggle.js

+	<svg className="components-form-toggle__on" width="2" height="6" aria-hidden="true" role="img" focusable="false" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 2 6"><path d="M0 0h2v6H0z" /></svg> : <svg className="components-form-toggle__off" width="6" height="6" aria-hidden="true" role="img" focusable="false" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 6 6"><path d="M3 1.5c.8 0 1.5.7 1.5 1.5S3.8 4.5 3 4.5 1.5 3.8 1.5 3 2.2 1.5 3 1.5M3 0C1.3 0 0 1.3 0 3s1.3 3 3 3 3-1.3 3-3-1.3-3-3-3z" /></svg>
+	}
+		</span>
+);


Please check indentation from line 24 to line 31

afercia · 2018-07-19T14:38:48Z

composites/Plugin/CornerstoneContent/components/CornerstoneToggle.js

+`;
+
+class CornerstoneToggle extends React.Component {
+


eslint "warning Block must not be padded by blank lines", please remove this blank line

afercia · 2018-07-19T14:39:44Z

composites/Plugin/Shared/components/GutenbergToggle.js

+import classnames from "classnames";
+import noop from "lodash/noop";
+
+function GutenbergToggle( { className, checked, id, onChange = noop, ...props } ) {


eslint error Missing JSDoc comment for this function
eslint error 'className' is missing in props validation
eslint error 'checked' is missing in props validation
eslint error 'id' is missing in props validation
eslint error 'onChange' is missing in props validation

afercia · 2018-07-19T14:41:20Z

composites/Plugin/Shared/components/GutenbergToggle.js

+
+function GutenbergToggle( { className, checked, id, onChange = noop, ...props } ) {
+	const wrapperClasses = classnames(
+		'components-form-toggle',


eslint error Strings must use doublequote

afercia · 2018-07-19T14:41:30Z

composites/Plugin/Shared/components/GutenbergToggle.js

+	const wrapperClasses = classnames(
+		'components-form-toggle',
+		className,
+		{ 'is-checked': checked },


eslint error Strings must use doublequote

afercia · 2018-07-19T14:42:30Z

composites/Plugin/Shared/components/GutenbergToggle.js

+				/>
+		<span className="components-form-toggle__track"></span>
+		<span className="components-form-toggle__thumb"></span>
+	{ checked ?


eslint error '?' should be placed at the beginning of the line

afercia · 2018-07-19T14:42:54Z

composites/Plugin/Shared/components/GutenbergToggle.js

+		<span className="components-form-toggle__track"></span>
+		<span className="components-form-toggle__thumb"></span>
+	{ checked ?
+	<svg className="components-form-toggle__on" width="2" height="6" aria-hidden="true" role="img" focusable="false" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 2 6"><path d="M0 0h2v6H0z" /></svg> : <svg className="components-form-toggle__off" width="6" height="6" aria-hidden="true" role="img" focusable="false" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 6 6"><path d="M3 1.5c.8 0 1.5.7 1.5 1.5S3.8 4.5 3 4.5 1.5 3.8 1.5 3 2.2 1.5 3 1.5M3 0C1.3 0 0 1.3 0 3s1.3 3 3 3 3-1.3 3-3-1.3-3-3-3z" /></svg>


eslint error Line 28 exceeds the maximum line length of 150

afercia · 2018-07-19T14:53:05Z

CR 🚧 please see comments

About associating the label: when clicking on the text the toggle should switch. Instead, nothing happens.

Since the label is not associated, the toggle (the underlying checkbox) is unlabelled and announced by screen readers just as "checkbox". In the rendered markup there's no "for" attribute:
<label>Cornerstone</label>

nor the checkbox has an ID. Looking at the props, there's a toggleId prop but it's not passed and in any case the label uses the key prop:
<label htmlFor={this.props.key}>Cornerstone</label>

while the checkbox uses the toggleId prop:
<GutenbergToggle ... id={this.props.toggleId}

Instead of passing this value as a prop, since the for/id attributes must always be set, I'd suggest to handle their values internally using lodash/uniqueId as we're already doing for other components.

afercia · 2018-07-19T14:54:41Z

composites/Plugin/CornerstoneContent/components/CornerstoneToggle.js

+			<Cornerstone>
+				<label htmlFor={this.props.key}>Cornerstone</label>
+				<GutenbergToggle
+					key={this.props.key}


not sure why there's the need of a key?

afercia · 2018-07-19T15:04:12Z

Note about accessibility;
on Gutenberg there's a long debate about these toggles, see WordPress/gutenberg#2146. The accessibility feedback asked for a way to have a textual indication of the toggle state. This is also what material.io recommends, when they state:

The option that the switch controls, as well as the state it’s in, should be made clear from the corresponding inline label.

We've suggested a couple alternative options:

1
indicating the state in the description below the label (this is currently used in Gutenberg for the Gallery crop setting):

2
using an "On/Off" test, as we're doing on MyYoast.

However, none of these 3 options is implemented in this component nor there's a way to change the inline label or use a description. I'd like to suggest this should be discussed, trying to find a good balance between accessibility and design. Of course, this can be addressed in a separate issue.
/Cc @hedgefield

hedgefield · 2018-07-24T08:19:31Z

@afercia For the Yoast sidebar, we intend to copy any components needed from their Gutenberg equivalents (if they exist). So the problem we face is really a problem we should fix in Gutenberg. I understand you'd rather not ship something not up to a11y standards, and neither do I, so let's build our version of this toggle to be compliant, and contribute that back as a PR at the same time. I'm a fan of the ON/OFF labels, they're simply the most efficient way of indicating state textually, I think we should try that first. Do you see any problems with this approach?

afercia · 2018-07-24T12:59:50Z

@hedgefield we already proposed the on/off text to the Gutenberg team, they don't like it. In the linked Gutenberg issues above we've also proposed different positions for the on/off (to the side or underneath the toggle) but still they don't like it.
Instead, the Gutenberg toggle already has it's needed to conditionally render the inline label or the help text below the label. Those can be used for the textual indication of the state, it's just a matter of implementation (and consensus).

hedgefield · 2018-07-24T13:17:41Z

@afercia That's too bad. Hmm. I'm not a fan of the help text, it adds a lot of bloat to the interface. But we can conditionally change the label text already? So let's do that. And then what do you think, use a variation on the original design and go for Mark/Unmark this as cornerstone content or This content has (not) been marked as cornerstone? (or something else)

maartenleenders · 2018-07-26T14:31:58Z

composites/Plugin/CornerstoneContent/components/CornerstoneToggle.js

+	}
+}
+
+CornerstoneToggle.propTypes = {


We usually set defaults for PropTypes that are not required.

… component

jcomack

CR done 🚧

Have some (minor) requests / questions.

jcomack · 2018-07-27T07:21:47Z

composites/Plugin/Shared/components/Toggle.js

+`;
+
+const ToggleVisualLabel = styled.span`
+	padding: 0px 0 0;


Seems odd to define 0px followed by two zeroes. I believe you can set all three to a simple 0 and be done with it.

Additionally, this means you're deliberately resetting all padding except for the one on the left-hand side. Not sure if this is intended or not.

jcomack · 2018-07-27T07:23:41Z

composites/Plugin/Shared/components/Toggle.js

+	margin: 0;
+	outline: 0;
+	&:focus > span {
+		box-shadow: inset 0 0 0 1px ${colors.$color_white}, 0 0 0 1px #5b9dd9, 0 0 2px 1px rgba(30, 140, 190, .8);


Unsure as to why the first color is variable, but the other two are not.

This applies also to "#a5d6a7" on line 8. When new colors are needed, please discuss with the design team: "our" colors should all be in the color palette (_colors.scss). Of course this makes sense when a new color is going to be used extensively. For colors that are used only once, we should wonder if they're really needed in the first place. Please consider this is essential to be able to keep our color palette under control in the long run. /Cc @hedgefield

In a way, #5b9dd9 and rgba(30, 140, 190, .8) are an exception, as they're the colors WordPress uses for the focus style. Normally, they're inherited on interactive elements. If we're going to use them extensively on non-natively interactive elements then we may want to consider to add them to the color palette. Worth noting Gutenberg uses slightly different colors 😬

jcomack · 2018-07-27T07:24:57Z

composites/Plugin/Shared/components/Toggle.js

+	}
+
+	/**
+	 * Returns the current enablement state.


enablement should be enabled

jcomack · 2018-07-27T07:25:04Z

composites/Plugin/Shared/components/Toggle.js

+	/**
+	 * Returns the current enablement state.
+	 *
+	 * @returns {boolean} The current enablement state.


See previous comment.

jcomack · 2018-07-27T07:25:22Z

composites/Plugin/Shared/components/Toggle.js

+	 * Sets the state to the opposite of the current state.
+	 *
+	 * @param {object} evt React SyntheticEvent.
+	 * @returns {void}


Missing whitespace above this line.

afercia · 2018-07-27T08:05:25Z

composites/Plugin/Shared/components/Toggle.js

+const ToggleBullet = styled.span`
+	background-color: ${ props => props.isEnabled ? colors.$color_green_medium_light : colors.$color_grey_medium_dark };
+	margin-left: ${ props => props.isEnabled ? "12px" : "-2px" };
+	box-shadow: 0 2px 2px 2px rgba(0, 0, 0, 0.1);


Same here: this is a hardcoded color. We should use a color from the palette (not necessarily a rgba color)

Hi Andrea, you are right. But we'll skip this for now to finish the component. We'll pick this up again during the polishing process.

afercia · 2018-07-27T08:08:32Z

composites/Plugin/Shared/components/Toggle.js

+					<ToggleBullet isEnabled={this.isEnabled()} />
+				</ToggleBar>
+				<ToggleVisualLabel aria-hidden="true">
+					{ this.isEnabled() ? __( "On", "yoast-components" ) : __( "Off", "yoast-components" ) }


if this component is going to be reusable, we should be able to pass a different visual label text.

Good point. We will make a issue next week to "improve" the toggle: refactor, change how we use colors and add props for the custom on/off label text.

afercia · 2018-07-27T08:09:11Z

composites/Plugin/Shared/components/Toggle.js

+	 * @param {object} evt React SyntheticEvent.
+	 * @returns {void}
+	 */
+	setEnabled( evt ) {


please avoid abbreviations: event is fine

afercia · 2018-07-27T08:10:05Z

composites/Plugin/Shared/components/Toggle.js

+					aria-checked={this.isEnabled()}
+					id={this.props.id}
+				>
+					<ToggleBullet isEnabled={this.isEnabled()} />


missing space within braces here and in the lines above

afercia · 2018-07-27T08:12:33Z

composites/Plugin/Shared/components/Toggle.js

+	 */
+	setEnabled( evt ) {
+		// Makes the toggle actionable with the Space bar key.
+		if ( evt.type === "keydown" && evt.which !== 32 ) {


I think keycode is safer than which (which is non-standard, though in jQuery is a normalized property in JavaScript it is not)

afercia · 2018-07-27T08:19:14Z

composites/Plugin/CornerstoneContent/components/CornerstoneToggle.js

+		return (
+			<Cornerstone>
+				<Toggle ariaLabel="Mark this post as cornerstone content" id="Cornerstone Toggle"
+				        labelText={__( "Mark this as cornerstone content.", "yoast-components" )} />


missing spaces between brackets

On a side note, we shouldn't use mixed spaces and tabs for indentation

afercia · 2018-07-27T08:23:02Z

composites/Plugin/CornerstoneContent/components/CornerstoneToggle.js

+	render() {
+		return (
+			<Cornerstone>
+				<Toggle ariaLabel="Mark this post as cornerstone content" id="Cornerstone Toggle"


the id should be lowercase and without spaces (it's used for HTML attributes). Also, it can't be hardcoded. IDs must be unique in a page, otherwise it's impossible to use more than one toggle.

True, in the toggle it is a prop we can set, but in the cornerstone toggle we keep it hard coded for now because there is only one cornerstone toggle.

…hub.com/Yoast/yoast-components into 10267-add-cornerstone-toggle-to-sidebar

boblinthorst · 2018-07-30T13:18:58Z

composites/Plugin/Shared/components/Toggle.js

+			<ToggleDiv>
+				{ /* eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-noninteractive-element-interactions */ }
+				<label
+					htmlFor={ this.props.id }


Not important enough for now, and also more a preference than an instruction.

but when props are used in multiple places, it's nice to extract props to local variables, this is done, for example, in SnippetEditorFields.js.

boblinthorst · 2018-07-30T13:20:22Z

CR: no important comments. but merge conflicts. 😞

Dieterrr and others added 5 commits July 17, 2018 15:20

temporary commit

cbe54a0

Added Gutenberg toggle and Cornerstone toggle

21dcb54

Added state manangement for now

5b583cf

Added tests

574b38b

Removed one test

bec7af7

Dieterrr mentioned this pull request Jul 19, 2018

10267 add cornerstone toggle to sidebar Yoast/wordpress-seo#10405

Closed

2 tasks

Fixed CS errors

8a15cf0

afercia reviewed Jul 19, 2018

View reviewed changes

Dieterrr added 3 commits July 23, 2018 13:54

Updated lodash, added components, added babel runtime, fixed CS

3e2531a

updated package.json

46bf775

Fixed CS

a332e47

Dieterrr and others added 6 commits July 25, 2018 19:52

added snapshot

442ff3b

Fixed CS

42110ff

Added Toggle from myYoast, replaced Gutenbergtoggle, added it to app.js

f243ce6

Made MyYoast toggle clickable by label, added prop for custom label

5dc58b2

Fixed CS errors

dbfce44

classnames dependency not needed

bb476f8

maartenleenders reviewed Jul 26, 2018

View reviewed changes

Dieterrr added 3 commits July 26, 2018 16:58

Fixed ESlint error, refactored to make code prettier. Added export of…

50bd497

… component

reverted changes

a546717

Merged with trunk, added default props

43a0d17

jcomack suggested changes Jul 27, 2018

View reviewed changes

afercia reviewed Jul 27, 2018

View reviewed changes

Dieterrr and others added 6 commits July 27, 2018 10:45

Implemented changes bases on CR

151386e

Refactored and added export

7a43c0e

Updated snap

d626b5a

Improved toggle code

e53ea6a

Merge branch '10267-add-cornerstone-toggle-to-sidebar' of https://git…

a17346c

…hub.com/Yoast/yoast-components into 10267-add-cornerstone-toggle-to-sidebar

Added missing props validation

224c201

boblinthorst reviewed Jul 30, 2018

View reviewed changes

Merged with develop

8b863c6

boblinthorst merged commit 4d14498 into develop Jul 30, 2018

boblinthorst deleted the 10267-add-cornerstone-toggle-to-sidebar branch July 30, 2018 14:50

abotteram added this to the 4.9.0 milestone Jul 31, 2018


		import GutenbergToggle from "../../Shared/components/GutenbergToggle";

		const Cornerstone = Styled.div`

10267 add cornerstone toggle to sidebar #668

10267 add cornerstone toggle to sidebar #668

Conversation

Dieterrr commented Jul 19, 2018 • edited by jcomack Loading

Summary

Relevant technical choices:

Test instructions

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 Jul 19, 2018 • edited Loading

Choose a reason for hiding this comment

afercia Jul 19, 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

afercia commented Jul 19, 2018

Choose a reason for hiding this comment

afercia commented Jul 19, 2018

hedgefield commented Jul 24, 2018

afercia commented Jul 24, 2018

hedgefield commented Jul 24, 2018

maartenleenders Jul 26, 2018 • edited Loading

Choose a reason for hiding this comment

jcomack left a 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

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 Jul 30, 2018 • edited Loading

Choose a reason for hiding this comment

boblinthorst commented Jul 30, 2018

Dieterrr commented Jul 19, 2018 •

edited by jcomack

Loading

afercia Jul 19, 2018 •

edited

Loading

afercia Jul 19, 2018 •

edited

Loading

maartenleenders Jul 26, 2018 •

edited

Loading

boblinthorst Jul 30, 2018 •

edited

Loading