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.

Sign up for GitHub

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

Wc 70 textinput pure component #246

Merged
merged 9 commits into from
Jul 10, 2017
Merged

Wc 70 textinput pure component #246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
dab2fe4
yarn
Jul 7, 2017
2ea5814
inherit from pure component and remove state
Jul 7, 2017
e957db2
change tests to not test state and value state
Jul 7, 2017
8fc19bf
functional component, tests breaking
Jul 10, 2017
b0858c9
wrap in stateful component
Jul 10, 2017
45b33b5
comments
Jul 10, 2017
b2abb9a
pull out into test utils
Jul 10, 2017
b6582cc
es6 syntax
Jul 10, 2017
bfd70c1
...other
Jul 10, 2017
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
122 changes: 54 additions & 68 deletions src/forms/TextInput.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,80 +5,66 @@ import cx from 'classnames';
/**
* @module TextInput
*/
class TextInput extends React.Component {
const TextInput = (props) => {

constructor(props) {
super(props);
this.state = {
value: props.value || '',
};
this.onChange = this.onChange.bind(this);
}
const {
name,
value,
label,
labelClassName,
className,
children,
error,
placeholder,
required,
id,
onChange,
isSearch,
maxLength,
pattern,
disabled,
...other
} = props;

onChange(e) {
this.setState({ value: e.target.value });
if (this.props.onChange) {
this.props.onChange(e);
}
}
const classNames = cx(
{ 'field--error': error },
className
);

render() {
const {
name,
value, // eslint-disable-line no-unused-vars
label,
labelClassName,
className,
children,
error,
placeholder,
required,
id,
onChange, // eslint-disable-line no-unused-vars
isSearch,
maxLength,
pattern,
disabled
} = this.props;
const labelClassNames = cx(
'label--field',
{ required, disabled },
labelClassName
);

const classNames = cx(
{ 'field--error': error },
className
);
return (
<div>
{label &&
<label className={labelClassNames} htmlFor={id}>
{label}
</label>
}
<input type={isSearch ? 'search' : 'text'}
name={name}
value={value}
required={required}
placeholder={placeholder}
className={classNames}
onChange={onChange}
maxLength={maxLength}
pattern={pattern}
disabled={disabled}
id={id}
{...other}
/>

const labelClassNames = cx(
'label--field',
{ required, disabled },
labelClassName
);
{ maxLength && <p className='text--caption align--right'>{value.length} / {maxLength}</p> }

return (
<div>
{label &&
<label className={labelClassNames} htmlFor={id}>
{label}
</label>
}
<input type={isSearch ? 'search' : 'text'}
name={name}
value={this.state.value}
required={required}
placeholder={placeholder}
className={classNames}
onChange={this.onChange}
maxLength={maxLength}
pattern={pattern}
disabled={disabled}
id={id} />

{ this.props.maxLength && <p className='text--caption align--right'>{this.state.value.length} / {this.props.maxLength}</p> }

{ error && <p className='text--error'>{error}</p> }
{children}
</div>
);
}
}
{ error && <p className='text--error'>{error}</p> }
{children}
</div>
);
};

TextInput.propTypes = {
name: PropTypes.string.isRequired,
Expand Down
85 changes: 33 additions & 52 deletions src/forms/textInput.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import React from 'react';
import TestUtils from 'react-addons-test-utils';
import { TestWrapper } from '../utils/testUtils';
import TextInput from './TextInput';

describe('TextInput', function() {
Expand All @@ -21,12 +22,14 @@ describe('TextInput', function() {
required: true,
};
textInputComponent = TestUtils.renderIntoDocument(
Copy link

Choose a reason for hiding this comment

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

A renderComponent function might also be helpful for future updates... e.g. https://github.com/meetup/mup-web/blob/master/src/components/events/eventTimeDisplay.test.jsx#L19

Copy link
Contributor Author

Choose a reason for hiding this comment

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

since I won't be using that in these tests I'm reluctant to add it. I will when I do though!

<TextInput
name={NAME_ATTR}
label={LABEL_TEXT}
value={VALUE}
{...formAttrs}
/>
<TestWrapper>
<TextInput
name={NAME_ATTR}
label={LABEL_TEXT}
value={VALUE}
{...formAttrs}
/>
</TestWrapper>
);

inputEl = TestUtils.findRenderedDOMComponentWithTag(textInputComponent, 'input');
Expand Down Expand Up @@ -57,12 +60,14 @@ describe('TextInput', function() {

it('should have input type search if `isSearch` is set to true', () => {
textInputComponent = TestUtils.renderIntoDocument(
<TextInput
name={NAME_ATTR}
label={LABEL_TEXT}
value={VALUE}
isSearch
/>
<TestWrapper>
<TextInput
name={NAME_ATTR}
label={LABEL_TEXT}
value={VALUE}
isSearch
/>
</TestWrapper>
);

inputEl = TestUtils.findRenderedDOMComponentWithTag(textInputComponent, 'input');
Expand All @@ -72,12 +77,14 @@ describe('TextInput', function() {

it('should have a disabled attribute when specified', () => {
textInputComponent = TestUtils.renderIntoDocument(
<TextInput
name={NAME_ATTR}
label={LABEL_TEXT}
value={VALUE}
disabled
/>
<TestWrapper>
<TextInput
name={NAME_ATTR}
label={LABEL_TEXT}
value={VALUE}
disabled
/>
</TestWrapper>
);

inputEl = TestUtils.findRenderedDOMComponentWithTag(textInputComponent, 'input');
Expand All @@ -102,43 +109,17 @@ describe('TextInput', function() {
expect(inputEl.getAttribute('maxLength')).toEqual(MAX_LEN);
});

it('should set its value on input change', function() {
const newValue = `${VALUE}r`;
expect(inputEl.value).toEqual(VALUE);
TestUtils.Simulate.change(inputEl, { target: { value: newValue } });
expect(inputEl.value).toEqual(newValue);
});

it('should call onChange and setState with input change', function() {
const newValue = `${VALUE}r`;
const changeSpy = spyOn(TextInput.prototype, 'onChange').and.callThrough();
const stateSpy = spyOn(TextInput.prototype, 'setState').and.callThrough();

const boundComponent = TestUtils.renderIntoDocument(
<TextInput
name={NAME_ATTR}
label={LABEL_TEXT}
value={VALUE}
onChange={onChange}
/>
);

inputEl = TestUtils.findRenderedDOMComponentWithTag(boundComponent, 'input');
TestUtils.Simulate.change(inputEl, { target: { value: newValue } });

expect(changeSpy).toHaveBeenCalled();
expect(stateSpy).toHaveBeenCalledWith({ value: newValue });
});

it('should call onChange `props` function when input is changed', () => {
const newValue = `${VALUE}r`;
const boundComponent = TestUtils.renderIntoDocument(
<TextInput
name={NAME_ATTR}
label={LABEL_TEXT}
value={VALUE}
onChange={onChange}
/>
<TestWrapper>
<TextInput
name={NAME_ATTR}
label={LABEL_TEXT}
value={VALUE}
onChange={onChange}
/>
</TestWrapper>
);
inputEl = TestUtils.findRenderedDOMComponentWithTag(boundComponent, 'input');
TestUtils.Simulate.change(inputEl, { target: { value: newValue } });
Expand Down
11 changes: 11 additions & 0 deletions src/utils/testUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import React from 'react';
import createReactClass from 'create-react-class';
import TestUtils from 'react-addons-test-utils';

export const variantTest = (FoundationComponent, className, variants) => {
Expand Down Expand Up @@ -32,3 +33,13 @@ export const hasRoleAttribute = (el, roleName) => {
export const componentHasProperty = (component, prop, value) => {
expect(component && component.props && component.props[prop] === value).toBe(true);
};

// Note: functional components need to be
// wrapped it in a stateful component to use TestUtils effectively
export const TestWrapper = createReactClass({
render: function() {
Copy link
Contributor

@mmcgahan mmcgahan Jul 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

in general the preferred solution would be to use enzyme instead of TestUtils.renderIntoDocument in these cases - I think it's fine to add this helper for 'legacy' code, but I'd like to see a @deprecated code comment and (ideally) a console.warn('Using deprecated TestWrapper for TestUtils.renderIntoDocument - covert test to use enzyme') as a loud flag about the tech debt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok so - how do we start using enzyme? we can do this in a separate branch and I can make a ticket for that. if you want I can add the deprecated comment but I dont know if that really helps?... as of this tool set and library this code is valid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://meetup.atlassian.net/browse/WC-72

Copy link
Contributor

Choose a reason for hiding this comment

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

@unjust I can go over some of the enzyme stuff with you, but I can't remember who's on your team - would @eilinora or @sadafie be able to give you a quick rundown?

For that WC-72 ticket, could you add a note about removing TestWrapper alongside those changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure no problem. steve and I are on the team. I can do some reading - I just would like to do this upgrade/changes in a separate ticket to make sure it corrects this problem. I'm also checking in with adam to make sure he knows.

return (
<div>{this.props.children}</div>
);
}
});
4 changes: 2 additions & 2 deletions yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3685,14 +3685,14 @@ js-tokens@^3.0.0:
version "3.0.1"
resolved "https://registry.yarnpkg.com/js-tokens/-/js-tokens-3.0.1.tgz#08e9f132484a2c45a30907e9dc4d5567b7f114d7"

[email protected], js-yaml@^3.4.3, js-yaml@^3.5.1:
[email protected], js-yaml@^3.5.1:
version "3.6.1"
resolved "https://registry.yarnpkg.com/js-yaml/-/js-yaml-3.6.1.tgz#6e5fe67d8b205ce4d22fad05b7781e8dadcc4b30"
dependencies:
argparse "^1.0.7"
esprima "^2.6.0"

js-yaml@^3.7.0, js-yaml@~3.7.0:
js-yaml@^3.4.3, js-yaml@^3.7.0, js-yaml@~3.7.0:
version "3.7.0"
resolved "https://registry.yarnpkg.com/js-yaml/-/js-yaml-3.7.0.tgz#5c967ddd837a9bfdca5f2de84253abe8a1c03b80"
dependencies:
Expand Down