Implements the snippet editor in React #460

atimmer · 2018-04-05T10:31:41Z

Summary

This PR can be summarized i.n the following changelog entry:

Add a SnippetEditor component. Renders the SnippetPreview component with editor fields to change the data.
Add a ReplacementVariable component. Renders a DraftJS editor with replacement variables as entities.
Add a FormattedScreenReaderMessage component Renders a message translated, but also as screen reader text. Ensures that ScreenReaderText can remain pure and only accept a string.

Relevant technical choices:

DraftJS is used for the input fields.
- The DraftJS mention plugin is used to auto complete replacement variables.
- % is used as a trigger.
- Replacement variables need to be passed as key value pairs. The actual shape is abstracted in a constants.js file.
- We serialize the DraftJS editor state to a string on every change. Because these are small input field this is feasible. If performance suffers we need to serialize less often.
- DraftJS is abstracted to a ReplacementVariableEditor component. This expects a string and will manage the DraftJS editor state.
SnippetEditor is the main component.
- It contains state that doesn't need to travel up the tree. Such as the hovered and active fields. And whether the editor is open.
- The data travels upwards and is accepted as props.
The progress bars are not measured yet. @herregroen and I thought we should solve this in YoastSEO.js. Because the assessment should be able to run independently of the snippet preview. The SnippetEditor component accepts the shape of an assessment result which the progress bars are rendered based upon.
The default mode is mobile.
We've dropped the SnippetPreview example because the SnippetEditor example covers that completely.
Added 3 colors to colors.scss and colors.json that are used for the carets.

Also includes some quality of life changes

Change devtool to cheap-module-eval-source-map which make sourcemaps work.
Add NameModulesPlugin so you can see which modules have been hot reloaded.
Use .babelrc in the webpack config.
Add CaseSensitivePathsPlugin, this makes us aware of case sensitivity issues earlier.
Adds new pattern for exposing public components. Every separate part can expose its components using an index.js, these will be aggregated in the root index.js. I haven't refactored all other components.
Setup test coverage for all files. This means now files that have 0% coverage are also included in the coverage. This drastically reduced the coverage, but it does mean that it is more accurate.

Test instructions

This PR can be tested by following these steps:

Turn on the local example using yarn start.
Go to the Snippet preview menu item.
Test that the snippet editor works in the same way as the old one (currently in the plugin).

moorscode · 2018-04-05T11:46:07Z

App.js

@@ -13,6 +12,7 @@ import SidebarCollapsibleWrapper from "./app/SidebarCollapsibleWrapper";
 // Required to make Material UI work with touch screens.
 import injectTapEventPlugin from "react-tap-event-plugin";
 import Checkbox from "./composites/Plugin/Shared/components/Checkbox";
+import SnippetEditor from "./app/SnippetEditorWrapper";


Wrapper naming does not seem to be explanatory, have you considered using Example?

herregroen · 2018-04-05T11:47:18Z

App.js

@@ -2,7 +2,6 @@ import React from "react";
 import { IntlProvider } from "react-intl";

 import SearchResultsEditor from "./composites/SearchResultEditor/SearchResultEditor";


imports should be sorted and separated by type. Currently composites and app components are mixed.

herregroen · 2018-04-05T11:50:01Z

a11y/FormattedScreenReaderMessage.js

@@ -0,0 +1,35 @@
+import React from "react";


Internal and external dependencies should be separated. ScreenReaderText should likely be imported at the very bottom with a newline between it and the external dependencies.

herregroen · 2018-04-05T11:52:06Z

a11y/FormattedScreenReaderMessage.js

+ * punctuation can for example be a colon.
+ *
+ * @param {Object} props The view properties.
+ * @param {string} props.before A piece of text to render before the translation.


Clearly specifying the keys this object accepts helps a lot in understanding what's going on.

herregroen · 2018-04-05T11:53:26Z

a11y/FormattedScreenReaderMessage.js

+ */
+const FormattedScreenReaderMessage = ( props ) => {
+	return (
+		<FormattedMessage { ...omit( props, "children" ) }>


Using object deconstruction as opposed to the omit function likely is a lot better for performance.

herregroen · 2018-04-05T11:54:39Z

a11y/ScreenReaderText.js

@@ -1,5 +1,8 @@
+/* External dependencies */


Shouldn't these be single line comments?

And do we have any rules regarding comments having to end with a ..

herregroen · 2018-04-05T11:57:56Z

composites/Plugin/SnippetEditor/components/ReplacementVariableEditor.js

+import { EditorState, convertToRaw, convertFromRaw } from "draft-js";
+import Editor from "draft-js-plugins-editor";
+import createMentionPlugin, { defaultSuggestionsFilter } from "draft-js-mention-plugin";
+import { serializeEditor, unserializeEditor } from "../serialization";


Splitting internal and external dependencies here would also be helpful.

herregroen · 2018-04-05T12:02:30Z

composites/Plugin/SnippetEditor/components/ReplacementVariableEditor.js

+	serializeEditor,
+] );
+
+class ReplaceVarEditor extends React.Component {


This class is missing documentation.

herregroen · 2018-04-05T12:02:56Z

composites/Plugin/SnippetEditor/components/ReplacementVariableEditor.js

+	/**
+	 * Constructs the replacement variable editor for use.
+	 *
+	 * @param {Object} props The props to instantiate this editor with.


It would be nice to fully specify all the keys you are able to pass to props.

herregroen · 2018-04-05T12:10:11Z

composites/Plugin/SnippetEditor/components/ReplacementVariableEditor.js

+		this.onChange = this.onChange.bind( this );
+		this.onSearchChange = this.onSearchChange.bind( this );
+
+		this.mentionsPlugin = createMentionPlugin( {


Given that the mentions plugin is used for replacement variables it would probably be helpful to add a comment here why we're using the mentions plugin.

herregroen · 2018-04-05T12:11:09Z

composites/Plugin/SnippetEditor/components/SnippetEditor.js

+import { lengthAssessmentShape, replacementVariablesShape } from "../constants";
+import FormattedScreenReaderMessage from "../../../../a11y/FormattedScreenReaderMessage";
+
+const SwitcherButton = Button.extend`


This component should be documented and probably other styled components should be documented.

moorscode

Code Review - Dev Presentation feedback

moorscode · 2018-04-05T11:50:29Z

a11y/FormattedScreenReaderMessage.js

+ * punctuation can for example be a colon.
+ *
+ * @param {Object} props The view properties.
+ * @param {string} props.before A piece of text to render before the translation.


The formatting of this comment can be clearer, to make sure that the sub-props are visually also indented and belong below the parent.

moorscode · 2018-04-05T11:52:19Z

a11y/FormattedScreenReaderMessage.js

+ * @param {string} props.before A piece of text to render before the translation.
+ * @param {string} props.after A piece of text to render after the translation.
+ *
+ * @returns {ReactElement} ScreenReaderText The div containing the screen reader text.


ScreenReaderText seems redundant here.

moorscode · 2018-04-05T11:53:53Z

a11y/FormattedScreenReaderMessage.js

+const FormattedScreenReaderMessage = ( props ) => {
+	return (
+		<FormattedMessage { ...omit( props, "children" ) }>
+			{ ( textNodes ) => <ScreenReaderText>{ [ props.before, ...textNodes, props.after ].join( "" ) }</ScreenReaderText> }


This line is hard to read, please consider placing the elements on separate lines.

moorscode · 2018-04-05T11:58:34Z

composites/Plugin/SnippetEditor/components/ReplacementVariableEditor.js

+import { serializeEditor, unserializeEditor } from "../serialization";
+import flow from "lodash/flow";
+import PropTypes from "prop-types";
+import { replacementVariablesShape } from "../constants";


As this is a constant, having a visual representation would be nice. In other languages constants are normally defined as all-caps.

moorscode · 2018-04-05T12:02:10Z

composites/Plugin/SnippetEditor/components/ReplacementVariableEditor.js

+ *
+ * @returns {EditorState} The editor state.
+ */
+const createEditorState = flow( [


flow is a magical thing, I cannot really guess what is happening here. Is there a way to make this more usable/readable?

After you know what flow does, this code is more readable than its alternatives.

moorscode · 2018-04-05T12:04:02Z

composites/Plugin/SnippetEditor/components/ReplacementVariableEditor.js

+
+ReplaceVarEditor.propTypes = {
+	content: PropTypes.string.isRequired,
+	onChange: PropTypes.func,


Consider placing this close to the other event-props.

moorscode · 2018-04-05T12:04:25Z

composites/Plugin/SnippetEditor/components/ReplacementVariableEditor.js

+
+ReplaceVarEditor.propTypes = {
+	content: PropTypes.string.isRequired,
+	onChange: PropTypes.func,


This is not required, but also not supplied in the defaultProps.

moorscode · 2018-04-05T12:06:19Z

composites/Plugin/SnippetEditor/components/ReplacementVariableEditor.js

+			replacementVariables: props.replacementVariables,
+		};
+
+		this._serializedContent = content;


The sudden appearance of serialized is not evident from the code, could you elaborate on this in a comment or maybe choose different variable names.

Suggestions: use raw as definition, as this is unprepared/touched.

atimmer · 2018-04-05T11:44:40Z

App.js

@@ -13,6 +12,7 @@ import SidebarCollapsibleWrapper from "./app/SidebarCollapsibleWrapper";
 // Required to make Material UI work with touch screens.
 import injectTapEventPlugin from "react-tap-event-plugin";
 import Checkbox from "./composites/Plugin/Shared/components/Checkbox";
+import SnippetEditor from "./app/SnippetEditorWrapper";


Move import to all the other imports that are examples.

atimmer · 2018-04-05T11:45:41Z

App.js

@@ -2,7 +2,6 @@ import React from "react";
 import { IntlProvider } from "react-intl";


Comment about examples

atimmer · 2018-04-05T11:48:06Z

a11y/FormattedScreenReaderMessage.js

+import ScreenReaderText from "./ScreenReaderText";
+import { FormattedMessage } from "react-intl";
+import omit from "lodash/omit";
+import PropTypes from "prop-types";


Why is this with a capital letter?

atimmer · 2018-04-05T11:54:09Z

a11y/ScreenReaderText.js

@@ -1,5 +1,8 @@
+/* External dependencies */


igorschoester

CR 🚧

igorschoester · 2018-04-10T08:11:44Z

app/SnippetEditorExample.js

 import styled from "styled-components";
+import debounce from "lodash/debounce";
+
+// External dependencies.


Should be internal dependencies here.

igorschoester · 2018-04-10T08:12:06Z

a11y/ScreenReaderText.js

 import React from "react";
 import PropTypes from "prop-types";
+
+// External dependencies.


Should be internal dependencies here.

igorschoester · 2018-04-10T08:12:14Z

a11y/FormattedScreenReaderMessage.js

+import omit from "lodash/omit";
+import PropTypes from "prop-types";
+
+// External dependencies.


Should be internal dependencies here.

igorschoester · 2018-04-10T08:27:15Z

app/SnippetEditorExample.js

 	 */
 	render() {
-		let props = Object.assign( {}, this.state );
+		const data = {


This is separate for readability? If yes, then why did you not extend this to titleLengthAssessment and descriptionLengthAssessment also?

igorschoester · 2018-04-10T09:18:27Z

app/SnippetEditorExample.js


-		props.onMouseOver = this.onMouseOver;
-		props.onMouseLeave = this.onMouseLeave;
+		let props = Object.assign( {}, this.state, {


To avoid confusion maybe not name this props. Reminds me of the eslint no-shadow setting we added before, that is not yet live.
Also this can be const instead of let.

igorschoester · 2018-04-10T10:49:19Z

composites/Plugin/SnippetEditor/components/SnippetEditor.js

+	 * @returns {void}
+	 */
+	setFieldFocus( field ) {
+		field = this.mapFieldToEditor( field );


Why is this on it's own line?

Because I find it easier to read.

igorschoester · 2018-04-10T10:55:21Z

composites/Plugin/SnippetEditor/components/SnippetEditorFields.js

+	/**
+	 * Constructs the snippet editor fields.
+	 *
+	 * @param {Object} props The props for the editor fields.


Maybe add propTypes descriptions?

igorschoester · 2018-04-10T11:05:55Z

composites/Plugin/SnippetPreview/components/ProgressBar.js

@@ -1,5 +1,8 @@
+/* External components */


Why components and not dependencies like everywhere else? Also multiline comment again.

igorschoester · 2018-04-10T11:08:14Z

composites/Plugin/SnippetEditor/components/SnippetEditorFields.js

@@ -0,0 +1,288 @@
+/* External components */


Why components here and not dependencies?

igorschoester · 2018-04-10T11:08:56Z

composites/Plugin/SnippetPreview/components/SnippetPreview.js

-/* Internal dependencies */
-import ScreenReaderText from "../../../../a11y/ScreenReaderText";
-import FixedWidthContainer from "./fixedWidthContainer";
+// External dependencies.


Should be internal dependencies here.

This gives us the ability to persist the content in the DraftJS editor.

This is a component that accepts replacement variables and renders a DraftJS editor for them. This way we can use it later in other preview editors.

This gives a view of the work that still has to be done.

This prevents duplication of configuration.

This makes sure that we don't do a lot of synchronous work.

This makes it possible to overwrite its styling from outside the component.

afercia · 2018-04-23T13:19:35Z

Turns out there are a few issues, some of them because wrong implementations in dra`ft.js and the mentions plugin. Other issues can be improved in our implementation.

Draft.js and mentions plugin issues:
the mention plugins passes the aria-expanded values as strings but then draft.js uses this pattern
var ariaExpanded = ariaRole === 'combobox' ? !!this.props.ariaExpanded : null;

so regardless the passed value is the string 'false' or 'true', it gets cast to boolean true anyways. The HTML attribute on the draft field is always true.

A combobox should have both an aria-haspopup and an aria-owns attributes, however in draft.js 0.10.2 they've made some changes:

* Change `aria-owns` to `aria-controls` in draft.js. ([@jessebeach](https://github.com/jessebeach) in [7f0cab28](https://github.com/facebook/draft-js/commit/7f0cab28386ec4bde8ec6289377bff9e53cd019b))
  * Deprecates support of `ariaOwns` and `ariaOwneeID` props.
* Deprecate use of `ariaHasPopup` prop in draft.js. `ariaExpanded` should be used instead if an input is showing a dropdown with options.([@jessebeach](https://github.com/jessebeach) in [744e9b4e](https://github.com/facebook/draft-js/commit/744e9b4eb4810797a93c66591fea6f9cac533b4b))

as a consequence, aria-haspopup and an aria-owns are missing from the combobox. Note that the mention plugin still passes the related props ariaHasPopup and ariaOwneeID but they're just ignored by draft.js. Note also the mention plugin dynamically changes the value of ariaHasPopup: that's wrong, as aria-haspopup is a property, not a state, and shouldn't change.

The highlighted option in the dropdown should get an aria-selected="true" attribute, this is completely missing in the mentions plugin and in draft.js (Safari and VoiceOver won't read out the highlighted option.

==========

Issues that can be improved in our implementation:

the mobile/desktop buttons should use aria-pressed, see how they work in the current snippet
the Edit snippet button aria-expanded, see how it works in the current snippet
when the suggestions are shown, there should be an audible message to announce the number of results, see for example how the autocompleters work in Gutenberg
when clicking the "Close snippet editor", focus should be moved back to the "Edit snippet" button

afercia · 2018-04-23T13:20:44Z

Acceptance 🚧 Please see comment above.

afercia · 2018-04-23T13:46:15Z

Visually, hovering/focusing the mobile icon makes it "jump".

afercia · 2018-04-23T14:03:24Z

Important: as far as I know, the slug field isn't meant to accept template variables, correct? In this case, it shouldn't be announced as combobox and shouldn't have any of the HTML attributes related to autocomplete, activedescendant. etc. It should be just an ordinary input field, and exposed as such to assistive technologies.

Positioning issues:
in some edge cases, the list of suggestions might be displayed off screen, for example when the caret in the "textarea" is close to the right edge. Not sure it the mentions plugin has anything to handle the positioning:

atimmer · 2018-04-23T14:32:42Z

Merging this. All the issues uncovered by Andrea I will create as issues to be fixed before release of the plugin.

afercia · 2018-04-23T19:59:02Z

I forgot to mention: there are no instructions on how to use the suggestions, e.g. type % and see what happens. I'd say even sighted user would need to be instructed. I'd suggest to try the simplest thing: add a paragraph below the fields type the percentage character % for a list of suggested template variables (or something along those lines) and target the paragraph with aria-describedby on the fields.

atimmer force-pushed the stories/snippet-editor branch from d7a8cf3 to 9fcf1fa Compare April 5, 2018 10:38

moorscode reviewed Apr 5, 2018

View reviewed changes

herregroen reviewed Apr 5, 2018

View reviewed changes

moorscode reviewed Apr 5, 2018

View reviewed changes

atimmer commented Apr 6, 2018

View reviewed changes

atimmer added the needs-code-review ★ label Apr 9, 2018

igorschoester added code-review ★ and removed needs-code-review ★ labels Apr 9, 2018

igorschoester self-assigned this Apr 9, 2018

igorschoester requested changes Apr 10, 2018

View reviewed changes

igorschoester added needs-action ★ and removed code-review ★ labels Apr 10, 2018

igorschoester assigned atimmer and unassigned igorschoester Apr 10, 2018

atimmer force-pushed the stories/snippet-editor branch 2 times, most recently from e9a5024 to c9bf2a8 Compare April 17, 2018 15:15

atimmer added needs-code-review ★ and removed needs-action ★ labels Apr 18, 2018

atimmer removed their assignment Apr 18, 2018

Herre Groen and others added 11 commits April 18, 2018 16:44

Introduce draft-js for snippet editor.

1f34314

Add DraftJS serialization functions

cdc7e0f

This gives us the ability to persist the content in the DraftJS editor.

Extract ReplaceVarEditor

bdadf05

This is a component that accepts replacement variables and renders a DraftJS editor for them. This way we can use it later in other preview editors.

Combine the snippet editor and the snippet preview

bbb858e

This gives a view of the work that still has to be done.

Change snippet example to show the editor

3637cbe

Add named colors for the snippet preview

e223c19

Make debugging with the dev server easier

c3ca833

Use babelrc in webpack config

84d46a7

This prevents duplication of configuration.

Connect the snippet preview with the snippet editor

d5843a9

Abstracts modes to the snippet preview file

0c3a2b9

Add localization

58406a1

igorschoester added the needs-acceptance ★ label Apr 19, 2018

igorschoester removed their assignment Apr 19, 2018

omarreiss mentioned this pull request Apr 20, 2018

Gutenberg integration roadmap Yoast/wordpress-seo#9189

Open

54 tasks

afercia added acceptance ★ and removed needs-acceptance ★ labels Apr 23, 2018

afercia self-assigned this Apr 23, 2018

atimmer added 4 commits April 23, 2018 14:55

Don't highlight an empty keyword

e9f7d00

Allow new content in the ReplacementVariableEditor

d8b2b8d

Delay fitting description

cfc8213

This makes sure that we don't do a lot of synchronous work.

Export StyledSectionBase

567f051

This makes it possible to overwrite its styling from outside the component.

afercia added queue ★ and removed acceptance ★ labels Apr 23, 2018

afercia assigned atimmer and unassigned afercia Apr 23, 2018

Improve the snippet editor accessibility

88011c3

Update snapshots after a11y changes

d37a06b

atimmer force-pushed the stories/snippet-editor branch from 8cf7f1e to d37a06b Compare April 23, 2018 14:27

atimmer merged commit 3c538af into develop Apr 23, 2018

atimmer deleted the stories/snippet-editor branch April 23, 2018 14:32

atimmer removed the needs-action ★ label Apr 23, 2018

atimmer added this to the 3.5.0 milestone Apr 24, 2018

atimmer mentioned this pull request May 1, 2018

Fix positioning of replacement variable suggestions Yoast/wordpress-seo#9604

Closed

		@@ -2,7 +2,6 @@ import React from "react";
		import { IntlProvider } from "react-intl";

		import SearchResultsEditor from "./composites/SearchResultEditor/SearchResultEditor";

Implements the snippet editor in React #460

Implements the snippet editor in React #460

Conversation

atimmer commented Apr 5, 2018 • edited

Summary

Relevant technical choices:

Also includes some quality of life changes

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moorscode 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

igorschoester 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

atimmer Apr 17, 2018 • edited

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 Apr 23, 2018

afercia commented Apr 23, 2018

afercia commented Apr 23, 2018

afercia commented Apr 23, 2018 • edited

atimmer commented Apr 23, 2018

afercia commented Apr 23, 2018

atimmer commented Apr 5, 2018 •

edited

atimmer Apr 17, 2018 •

edited

afercia commented Apr 23, 2018 •

edited