-
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
Implement input field component for keyword #482
Changes from 10 commits
ee64f21
727106f
79fab4a
a9f3d0d
f434857
ba9e730
1864aae
fe2686d
583da70
2f4aef4
21e3bcd
e0fd4c1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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"; | ||
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}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 !== "" ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could also do this in the JSX like:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After confirming with @IreneStr, we will no longer be using |
||
id="KeywordError" | ||
defaultMessage= "Are you trying to use multiple keywords? You should add them separately below." | ||
role="alert" | ||
/> | ||
</ErrorText> | ||
); | ||
} | ||
return ( | ||
<ErrorText /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be a |
||
KeywordField[ "border-color" ] = color; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ) } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
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" | ||
/>, | ||
] | ||
`; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See earlier comment about |
||
"enzyme-to-json": "^3.3.3", | ||
"eslint": "^4.16.0", | ||
"eslint-config-yoast": "^3.0.1", | ||
|
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.
Please separate external and internal imports.