-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implements the snippet editor in React #460
Conversation
d7a8cf3
to
9fcf1fa
Compare
App.js
Outdated
@@ -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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrapper
naming does not seem to be explanatory, have you considered using Example
?
@@ -2,7 +2,6 @@ import React from "react"; | |||
import { IntlProvider } from "react-intl"; | |||
|
|||
import SearchResultsEditor from "./composites/SearchResultEditor/SearchResultEditor"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imports should be sorted and separated by type. Currently composites
and app
components are mixed.
@@ -0,0 +1,35 @@ | |||
import React from "react"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
a11y/FormattedScreenReaderMessage.js
Outdated
* 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clearly specifying the keys this object accepts helps a lot in understanding what's going on.
a11y/FormattedScreenReaderMessage.js
Outdated
*/ | ||
const FormattedScreenReaderMessage = ( props ) => { | ||
return ( | ||
<FormattedMessage { ...omit( props, "children" ) }> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using object deconstruction as opposed to the omit
function likely is a lot better for performance.
a11y/ScreenReaderText.js
Outdated
@@ -1,5 +1,8 @@ | |||
/* External dependencies */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't these be single line comments?
And do we have any rules regarding comments having to end with a .
.
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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Splitting internal and external dependencies here would also be helpful.
serializeEditor, | ||
] ); | ||
|
||
class ReplaceVarEditor extends React.Component { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class is missing documentation.
/** | ||
* Constructs the replacement variable editor for use. | ||
* | ||
* @param {Object} props The props to instantiate this editor with. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to fully specify all the keys you are able to pass to props.
this.onChange = this.onChange.bind( this ); | ||
this.onSearchChange = this.onSearchChange.bind( this ); | ||
|
||
this.mentionsPlugin = createMentionPlugin( { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
import { lengthAssessmentShape, replacementVariablesShape } from "../constants"; | ||
import FormattedScreenReaderMessage from "../../../../a11y/FormattedScreenReaderMessage"; | ||
|
||
const SwitcherButton = Button.extend` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This component should be documented and probably other styled components should be documented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review - Dev Presentation feedback
a11y/FormattedScreenReaderMessage.js
Outdated
* 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The formatting of this comment can be clearer, to make sure that the sub-props are visually also indented and belong below the parent.
a11y/FormattedScreenReaderMessage.js
Outdated
* @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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ScreenReaderText
seems redundant here.
a11y/FormattedScreenReaderMessage.js
Outdated
const FormattedScreenReaderMessage = ( props ) => { | ||
return ( | ||
<FormattedMessage { ...omit( props, "children" ) }> | ||
{ ( textNodes ) => <ScreenReaderText>{ [ props.before, ...textNodes, props.after ].join( "" ) }</ScreenReaderText> } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is hard to read, please consider placing the elements on separate lines.
import { serializeEditor, unserializeEditor } from "../serialization"; | ||
import flow from "lodash/flow"; | ||
import PropTypes from "prop-types"; | ||
import { replacementVariablesShape } from "../constants"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this is a constant, having a visual representation would be nice. In other languages constants are normally defined as all-caps.
* | ||
* @returns {EditorState} The editor state. | ||
*/ | ||
const createEditorState = flow( [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
flow
is a magical thing, I cannot really guess what is happening here. Is there a way to make this more usable/readable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After you know what flow
does, this code is more readable than its alternatives.
|
||
ReplaceVarEditor.propTypes = { | ||
content: PropTypes.string.isRequired, | ||
onChange: PropTypes.func, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider placing this close to the other event-props.
|
||
ReplaceVarEditor.propTypes = { | ||
content: PropTypes.string.isRequired, | ||
onChange: PropTypes.func, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not required, but also not supplied in the defaultProps.
replacementVariables: props.replacementVariables, | ||
}; | ||
|
||
this._serializedContent = content; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestions: use raw
as definition, as this is unprepared/touched.
App.js
Outdated
@@ -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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move import to all the other imports that are examples.
@@ -2,7 +2,6 @@ import React from "react"; | |||
import { IntlProvider } from "react-intl"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment about examples
a11y/FormattedScreenReaderMessage.js
Outdated
import ScreenReaderText from "./ScreenReaderText"; | ||
import { FormattedMessage } from "react-intl"; | ||
import omit from "lodash/omit"; | ||
import PropTypes from "prop-types"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this with a capital letter?
a11y/ScreenReaderText.js
Outdated
@@ -1,5 +1,8 @@ | |||
/* External dependencies */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CR 🚧
app/SnippetEditorExample.js
Outdated
import styled from "styled-components"; | ||
import debounce from "lodash/debounce"; | ||
|
||
// External dependencies. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be internal dependencies here.
a11y/ScreenReaderText.js
Outdated
import React from "react"; | ||
import PropTypes from "prop-types"; | ||
|
||
// External dependencies. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be internal dependencies here.
a11y/FormattedScreenReaderMessage.js
Outdated
import omit from "lodash/omit"; | ||
import PropTypes from "prop-types"; | ||
|
||
// External dependencies. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be internal dependencies here.
*/ | ||
render() { | ||
let props = Object.assign( {}, this.state ); | ||
const data = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is separate for readability? If yes, then why did you not extend this to titleLengthAssessment and descriptionLengthAssessment also?
app/SnippetEditorExample.js
Outdated
|
||
props.onMouseOver = this.onMouseOver; | ||
props.onMouseLeave = this.onMouseLeave; | ||
let props = Object.assign( {}, this.state, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
* @returns {void} | ||
*/ | ||
setFieldFocus( field ) { | ||
field = this.mapFieldToEditor( field ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this on it's own line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I find it easier to read.
/** | ||
* Constructs the snippet editor fields. | ||
* | ||
* @param {Object} props The props for the editor fields. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add propTypes descriptions?
@@ -1,5 +1,8 @@ | |||
/* External components */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why components and not dependencies like everywhere else? Also multiline comment again.
@@ -0,0 +1,288 @@ | |||
/* External components */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why components here and not dependencies?
/* Internal dependencies */ | ||
import ScreenReaderText from "../../../../a11y/ScreenReaderText"; | ||
import FixedWidthContainer from "./fixedWidthContainer"; | ||
// External dependencies. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be internal dependencies here.
e9a5024
to
c9bf2a8
Compare
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.
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: 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
as a consequence, The highlighted option in the dropdown should get an ========== Issues that can be improved in our implementation:
|
Acceptance 🚧 Please see comment above. |
Visually, hovering/focusing the mobile icon makes it "jump". |
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: |
8cf7f1e
to
d37a06b
Compare
Merging this. All the issues uncovered by Andrea I will create as issues to be fixed before release of the plugin. |
I forgot to mention: there are no instructions on how to use the suggestions, e.g. type |
Summary
This PR can be summarized i.n the following changelog entry:
SnippetEditor
component. Renders theSnippetPreview
component with editor fields to change the data.ReplacementVariable
component. Renders a DraftJS editor with replacement variables as entities.FormattedScreenReaderMessage
component Renders a message translated, but also as screen reader text. Ensures thatScreenReaderText
can remain pure and only accept a string.Relevant technical choices:
DraftJS is used for the input fields.
%
is used as a trigger.constants.js
file.SnippetEditor
is the main component.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 theSnippetEditor
example covers that completely.Added 3 colors to
colors.scss
andcolors.json
that are used for the carets.Also includes some quality of life changes
cheap-module-eval-source-map
which make sourcemaps work..babelrc
in the webpack config.CaseSensitivePathsPlugin
, this makes us aware of case sensitivity issues earlier.index.js
, these will be aggregated in the rootindex.js
. I haven't refactored all other components.Test instructions
This PR can be tested by following these steps:
yarn start
.Snippet preview
menu item.