Implement input field component for keyword #482
Conversation
…implement-keyword-input-field-component
…implement-keyword-input-field-component
…implement-keyword-input-field-component
| import React from "react"; | ||
| import { FormattedMessage } from "react-intl"; | ||
| import styled from "styled-components"; | ||
| import colors from "../../../../style-guide/colors.json"; |
There was a problem hiding this comment.
Please separate external and internal imports.
|
|
||
| const ErrorText = styled.div` | ||
| font-size: 1em; | ||
| color: ${ colors.$color_red}; |
There was a problem hiding this comment.
Please add a space before the };
| /** | ||
| * Displays the error message | ||
| * | ||
| * @param {String} input The text of the input |
| ); | ||
| } | ||
| return ( | ||
| <ErrorText /> |
There was a problem hiding this comment.
Why still render this when there is no error?
| * @returns {Element} ErrorText The error message element | ||
| */ | ||
| displayErrorMessage( input = "" ) { | ||
| if ( this.state.showErrorMessage && input !== "" ) { |
There was a problem hiding this comment.
You could also do this in the JSX like:
return (
{ this.state.showErrorMessage && input !== "" && <ErrorText>
<FormattedMessage />
</ErrorText>
}
);
There was a problem hiding this comment.
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.
| /** | ||
| * Constructs a KeywordInput component | ||
| * | ||
| * @param {Object} props The props for this input field component. |
There was a problem hiding this comment.
I would love some extra documentation here on the props themselves (id, onChange, label and keyword).
|
|
||
| KeywordInput.propTypes = { | ||
| id: PropTypes.string.isRequired, | ||
| onChange: PropTypes.func, |
There was a problem hiding this comment.
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?
| { this.props.label } | ||
| </label> | ||
| <KeywordField type="text" id={ this.props.id } | ||
| onChange={ this.handleChange.bind( this ) } |
There was a problem hiding this comment.
Please bind this in the constructor, it only has to happen once, not on every render.
| if ( this.state.showErrorMessage && input !== "" ) { | ||
| return ( | ||
| <ErrorText> | ||
| <FormattedMessage |
There was a problem hiding this comment.
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!).
package.json
Outdated
| @@ -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.
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.
…plement changes mentioned in CR.
|
CR 👍 |
|
ACC 👍 |
Summary
This PR can be summarized in the following changelog entry:
Relevant technical choices:
Test instructions
This PR can be tested by following these steps:
yarn install,yarn start, go tohttp://localhost:3333/, and go to theKeywordtabFixes #466