Skip to content
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

Implement input field component for keyword #482

Merged
merged 12 commits into from
May 2, 2018
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions App.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import SnippetEditor from "./app/SnippetEditorExample";
// 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 KeywordInput from "./composites/Plugin/Shared/components/KeywordInput";

const components = [
{
Expand Down Expand Up @@ -62,6 +63,14 @@ const components = [
onChange={ event => console.log( event ) }
/>,
},
{
id: "focus-keyword",
name: "Keyword",
component: <KeywordInput
id="focus-keyword"
label={ "Focus keyword"}
/>,
},
{
id: "sidebar-collapsible",
name: "Sidebar Collapsible",
Expand Down
126 changes: 126 additions & 0 deletions composites/Plugin/Shared/components/KeywordInput.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
import React from "react";
import { FormattedMessage } from "react-intl";
import styled from "styled-components";
import colors from "../../../../style-guide/colors.json";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please separate external and internal imports.

import PropTypes from "prop-types";

let KeywordField = styled.input`
margin-right: 0.5em;
border-color: ${ props => props.borderColor };
`;

const ErrorText = styled.div`
font-size: 1em;
color: ${ colors.$color_red};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a space before the };

margin: 1em 0;
min-height: 1.8em;
`;

class KeywordInput extends React.Component {
/**
* Constructs a KeywordInput component
*
* @param {Object} props The props for this input field component.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would love some extra documentation here on the props themselves (id, onChange, label and keyword).

*
* @returns {void}
*/
constructor( props ) {
super( props );

this.state = {
showErrorMessage: false,
keyword: props.keyword,
};
}

/**
* Checks the keyword input for comma-separated words
*
* @param {String} keywordText The text of the input
*
* @returns {void}
*/
checkKeywordInput( keywordText ) {
let separatedWords = keywordText.split( "," );
if ( separatedWords.length > 1 ) {
this.setState( { showErrorMessage: true } );
} else {
this.setState( { showErrorMessage: false } );
}
}

/**
* Displays the error message
*
* @param {String} input The text of the input
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a lot of spaces 🙂

*
* @returns {Element} ErrorText The error message element
*/
displayErrorMessage( input = "" ) {
if ( this.state.showErrorMessage && input !== "" ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also do this in the JSX like:

return (
    { this.state.showErrorMessage && input !== "" && <ErrorText>
             <FormattedMessage />
         </ErrorText>
    }
);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to not use this in the end, since I changed the function to not render anything when there is no error, and I find the explicit function conditional I have to be more clear. Happy to change it though in case that would be more inline with other component's code style.

return (
<ErrorText>
<FormattedMessage
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After confirming with @IreneStr, we will no longer be using react-intl in the future. Since you are explicitly adding the enzyme-react-intl package to test this and we might forget to remove this later. Please remove this component and test package (but still use role="alert" of course!).

id="KeywordError"
defaultMessage= "Are you trying to use multiple keywords? You should add them separately below."
role="alert"
/>
</ErrorText>
);
}
return (
<ErrorText />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why still render this when there is no error?

);
}

/**
* Handles changes in the KeywordInput.
*
* @param {Event} event The onChange event.
*
* @returns {void} Calls the checkKeywordInput-function.
*/
handleChange( event ) {
this.setState( { keyword: event.target.value } );
this.checkKeywordInput( event.target.value );
}

/**
* Renders an input field, a label, and if the condition is met, an error message.
*
* @returns {ReactElement} The KeywordField react component including its label and eventual error message.
*/
render() {
let color = this.state.showErrorMessage ? "red" : "white";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a const.

KeywordField[ "border-color" ] = color;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this do?

return(
<React.Fragment>
<label htmlFor={ this.props.id }>
{ this.props.label }
</label>
<KeywordField type="text" id={ this.props.id }
onChange={ this.handleChange.bind( this ) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please bind this in the constructor, it only has to happen once, not on every render.

borderColor={ color }
/>
{ this.displayErrorMessage( this.state.keyword ) }
</React.Fragment>
);
}
}

KeywordInput.propTypes = {
id: PropTypes.string.isRequired,
onChange: PropTypes.func,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see this prop (onChange) actually being used anywhere. Is this something that you where planning on taking outside of this component before?

label: PropTypes.oneOfType( [
PropTypes.string,
PropTypes.array,
] ).isRequired,
keyword: PropTypes.string,
};

KeywordInput.defaultProps = {
keyword: "",
};

export default KeywordInput;

56 changes: 56 additions & 0 deletions composites/Plugin/Shared/tests/KeywordInputTest.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
import React from "react";
import renderer from "react-test-renderer";
import { mountWithIntl } from "enzyme-react-intl";

import KeywordInput from "../components/KeywordInput";

describe( KeywordInput, () => {
it( "matches the snapshot by default", () => {
const component = renderer.create(
<KeywordInput id="test-id" onChange={ () => {
} } label="test label"/>
);

let tree = component.toJSON();
expect( tree ).toMatchSnapshot();
} );

it( "does not display the error message for a single keyword", () => {
const wrapper = mountWithIntl(
<KeywordInput id="test-id" label="test label" />
);
wrapper.find( "input" ).simulate( "change", {
target: {
value: "Keyword1",
},
} );
expect( wrapper.state().showErrorMessage ).toEqual( false );
expect( wrapper.find( "div" ).text() ).toBeFalsy();
} );

it( "does not display the error message for two words separated by whitespace", () => {
const wrapper = mountWithIntl(
<KeywordInput id="test-id" label="test label" />
);
wrapper.find( "input" ).simulate( "change", {
target: {
value: "Keyword1 Keyword2",
},
} );
expect( wrapper.state().showErrorMessage ).toEqual( false );
expect( wrapper.find( "div" ).text() ).toBeFalsy();
} );

it( "displays the error message for comma-separated words", () => {
const wrapper = mountWithIntl(
<KeywordInput id="test-id" label="test label" />
);
wrapper.find( "input" ).simulate( "change", {
target: {
value: "Keyword1, Keyword2",
},
} );
expect( wrapper.state().showErrorMessage ).toEqual( true );
expect( wrapper.find( "div" ).text() ).toBeTruthy();
} );
} );
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`KeywordInput matches the snapshot by default 1`] = `
Array [
<label
htmlFor="test-id"
>
test label
</label>,
.c0 {
margin-right: 0.5em;
border-color: white;
}

<input
className="c0"
id="test-id"
onChange={[Function]}
type="text"
/>,
.c0 {
font-size: 1em;
color: #dc3232;
margin: 1em 0;
min-height: 1.8em;
}

<div
className="c0"
/>,
]
`;
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@
"concurrently": "^3.5.0",
"enzyme": "^3.3.0",
"enzyme-adapter-react-16": "^1.1.1",
"enzyme-react-intl": "^2.0.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See earlier comment about react-intl. And for future notice, please also commit yarn.lock after changing something in package.json or already if some minor version got updated.

"enzyme-to-json": "^3.3.3",
"eslint": "^4.16.0",
"eslint-config-yoast": "^3.0.1",
Expand Down