Wc 70 textinput pure component #246

unjust · 2017-07-07T19:01:27Z

Related issues

Fixes https://meetup.atlassian.net/browse/WC-70
related to https://meetup.atlassian.net/browse/WC-69

Description

Lets make our inputs and form fields PureComponents. This way they will play well with Redux forms in the app as well.

Screenshots (if applicable)

mikespencer · 2017-07-07T20:24:04Z

src/forms/TextInput.jsx

 		this.onChange = this.onChange.bind(this);
 	}

 	onChange(e) {
-		this.setState({ value: e.target.value });
 		if (this.props.onChange) {
 			this.props.onChange(e);


If you attached this directly to the onChange prop of the input, you could really trim down a lot of the code in this component:

<input onChange={this.props.onChange} ... />

You'd be able to delete the constructor and onChange methods altogether while keeping the same functionality. You could then even go one step further and define this as a stateless functional component instead of a class if you wanted (no need to worry about this or lifecycle methods):

class TextInput extends React.PureComponent { render() { const { /* destructure props */ } = this.props; return <input ... /> } } // vs function TextInput({/* destructure props */}) { return <input ... /> }

hm ok. Well what do we prefer? PureComponent vs stateless functional component? What are the benefits?
@eilinora should we go for the latter instead of a PureComponent?

I agree with @mikespencer we should be trying to go for stateless functional components as much as possible. Especially at the MWC level. I think my only concern is how this change would interact if we wanted this component to work with redux forms. @mikespencer any concerns there?

I think my only concern is how this change would interact if we wanted this component to work with redux forms. @mikespencer any concerns there?

Ah right - so this won't work as is with redux-form. Redux-form passes in props in a specific way to the rendered component. I like the idea of having this component being purely presentational though with no ties to a specific library, like redux-form.

One way I'd love to see this work would be to wrap this in another component that is meant for consumption by redux-form, roughly something like:

import TextInput from 'meetup-web-components'; const ReduxFormTextInput = function ReduxFormTextInput(props) { const translatedProps = { // convert the props that redux-form passes into this component into something that TextInput understands } return <TextInput {...translatedProps} />; }

That way, this TextInput can be used in different situations purely for presentation and is totally decoupled from redux-form

unjust · 2017-07-10T15:40:57Z

I made this a functional component but the most basic tests (eg it exists) are even failing. Stroybook looks good. Any ideas how the test needs to change @mikespencer

unjust · 2017-07-10T16:34:02Z

Tests need to be changed since this is a functional component, and TestUtils.renderIntoDocument returns null for these tyes of components
"NOTE:
Because stateless functions don't have a backing instance, you can't attach a ref to a stateless function component. Normally this isn't an issue, since stateless functions do not provide an imperative API. Without an imperative API, there isn't much you could do with an instance anyway. However, if a user wants to find the DOM node of a stateless function component, they must wrap the component in a stateful component (eg. ES6 class component) and attach the ref to the stateful wrapper component."
facebook/react#5455 (comment)

My fix is to create a Wrapper class, like this example:
facebook/react#4692 (comment)

eilinora · 2017-07-10T16:39:57Z

src/forms/TextInput.jsx

@@ -5,79 +5,62 @@ import cx from 'classnames';
 /**
 * @module TextInput
 */
-class TextInput extends React.Component {
+function TextInput(props) {


can i be obnoxious and ask this to be written es6'y?
const TextInput = props => { ... };

happy to oblige

One thing to note with doing it this way (anonymous function) is that it can be trickier to figure out what the problem is if you are looking at an error's call stack. Personally, I prefer to reference airbnb's style guide where they recommend explicitly naming the function in these sort of situations. Not a major thing, but something to be aware of

I was thinking the same thing. Torn between es6 syntax and a named function. @ivy @eilinora

Hmmm, yeah we've been doing it the es6 way as we've been migrating to stateless. I see the style guide says boo to it. I vote for the es6, but I'll defer to the collective on which way we go. 😃

I recommend using an arrow function + static .displayName property or a named function, but not a 'bare' arrow function without a name.

eilinora · 2017-07-10T16:41:41Z

src/forms/textInput.test.jsx

@@ -10,6 +11,16 @@ describe('TextInput', function() {
 		MAX_LEN = '20',
 		ERROR_TEXT = 'Too wimpy.';

+	// Note: since TextInput is a functional component
+	// we need to wrap it in a stateful component to use TestUtils effectively
+	const TestWrapper = createReactClass({


might be good to make this a utility as we move other MWC components to stateless. i suggest in testUtils

eilinora · 2017-07-10T16:45:00Z

src/forms/textInput.test.jsx

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


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

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

eilinora

Only change i'm gonna say is a blocker is my obnoxious es6 comment https://github.com/meetup/meetup-web-components/pull/246/files#r126474675. Everything else is nice to haves.

mikespencer · 2017-07-10T18:24:57Z

I just realised I left a comment in a collapsed comment, so in case it gets missed I'm just going to paste it here:

I think my only concern is how this change would interact if we wanted this component to work with redux forms. @mikespencer any concerns there?

Ah right - so this won't work as is with redux-form. Redux-form passes in props in a specific way to the rendered component. I like the idea of having this component being purely presentational though with no ties to a specific library, like redux-form.

One way I'd love to see this work would be to wrap this in another component that is meant for consumption by redux-form, roughly something like:

import TextInput from 'meetup-web-components';

const ReduxFormTextInput = function ReduxFormTextInput(props) {
  const translatedProps = {
    // convert the props that redux-form passes into this component into something that TextInput understands
  }
  return <TextInput {...translatedProps} />;
}

That way, this TextInput can be used in different situations purely for presentation and is totally decoupled from redux-form

EDIT: We could even leverage mapProps from recompose for this, so then you wouldn't need to create a component or custom higher order component to map the redux-form props to this TextInput's expected props

unjust · 2017-07-10T18:32:54Z

ok @mikespencer, so the ReduxFormTextInput or something like that would live in mup-web correct?

if this review is good to go, I'll merge - just let me know!

mikespencer · 2017-07-10T18:42:43Z

ok @mikespencer, so the ReduxFormTextInput or something like that would live in mup-web correct?
if this review is good to go, I'll merge - just let me know!

Yeah I think that one could be debated - if the goal of meetup-web-components is to be purely presentational, then I'd say it's probably best not to have components coupled to redux-form inside here, since it assumes the consuming application is using redux-form. Although, if the majority of the repos consuming this meetup-web-components do use redux-form, then having a redux-form input component in here could be an easy easy way to share between applications. I guess an ideal approach would be to have another package for meetup-redux-form-components that basically wrap meetup-web-components to make them compatible with redux-form, but that's just my opinion!

Either way, I think all of the above are out of the scope of this PR, so I'm 👍 with this.

eilinora · 2017-07-10T18:51:51Z

Oh that brings up a good point, there should be a ...other in here on the input in case we need to pass in something this component was initially setup to support. (.e.g. if we did want to manage state of the input at the form level with an onBlur or something..., etc.

mmcgahan · 2017-07-10T19:01:15Z

src/utils/testUtils.js

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


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

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.

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

@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?

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.

unjust · 2017-07-10T19:21:05Z

merging, stay tuned for enzyme

ivy added 3 commits July 7, 2017 12:31

yarn

dab2fe4

inherit from pure component and remove state

2ea5814

change tests to not test state and value state

e957db2

unjust requested review from eilinora and mikespencer July 7, 2017 19:16

mikespencer reviewed Jul 7, 2017

View reviewed changes

functional component, tests breaking

8fc19bf

wrap in stateful component

b0858c9

comments

45b33b5

eilinora reviewed Jul 10, 2017

View reviewed changes

eilinora suggested changes Jul 10, 2017

View reviewed changes

ivy added 2 commits July 10, 2017 12:51

pull out into test utils

b2abb9a

es6 syntax

b6582cc

...other

bfd70c1

mmcgahan reviewed Jul 10, 2017

View reviewed changes

eilinora approved these changes Jul 10, 2017

View reviewed changes

unjust merged commit d87941e into master Jul 10, 2017

chenrui333 deleted the WC-70_textinput_pure_component branch September 16, 2019 22:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wc 70 textinput pure component #246

Wc 70 textinput pure component #246

unjust commented Jul 7, 2017

mikespencer Jul 7, 2017

unjust Jul 10, 2017

eilinora Jul 10, 2017

mikespencer Jul 10, 2017

unjust commented Jul 10, 2017

unjust commented Jul 10, 2017

eilinora Jul 10, 2017

unjust Jul 10, 2017

mikespencer Jul 10, 2017

aquillano Jul 10, 2017

eilinora Jul 10, 2017

mmcgahan Jul 10, 2017

eilinora Jul 10, 2017

eilinora Jul 10, 2017

unjust Jul 10, 2017

eilinora left a comment

mikespencer commented Jul 10, 2017 •

edited

Loading

unjust commented Jul 10, 2017

mikespencer commented Jul 10, 2017

eilinora commented Jul 10, 2017 •

edited

Loading

mmcgahan Jul 10, 2017 •

edited

Loading

unjust Jul 10, 2017

unjust Jul 10, 2017

mmcgahan Jul 10, 2017

unjust Jul 10, 2017

unjust commented Jul 10, 2017

Wc 70 textinput pure component #246

Wc 70 textinput pure component #246

Conversation

unjust commented Jul 7, 2017

Related issues

Description

Screenshots (if applicable)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unjust commented Jul 10, 2017

unjust commented Jul 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eilinora left a comment

Choose a reason for hiding this comment

mikespencer commented Jul 10, 2017 • edited Loading

unjust commented Jul 10, 2017

mikespencer commented Jul 10, 2017

eilinora commented Jul 10, 2017 • edited Loading

mmcgahan Jul 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unjust commented Jul 10, 2017

mikespencer commented Jul 10, 2017 •

edited

Loading

eilinora commented Jul 10, 2017 •

edited

Loading

mmcgahan Jul 10, 2017 •

edited

Loading