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

InputText value & defaultValue Problem. #80

Closed
serayuzgur opened this issue Feb 4, 2016 · 24 comments
Closed

InputText value & defaultValue Problem. #80

serayuzgur opened this issue Feb 4, 2016 · 24 comments

Comments

@serayuzgur
Copy link

Summary: value attr should be taken from state. I am planning to change it that way and implement componentShouldUpdate

Here is the scenario. I have a grid.

  1. Selection + Edit button renders a form.
  2. Form must be rendered with the values from selection.
  3. Form must be able to re-render it self if we do 1st step again (change selection & click edit)
  4. Form values must be editable after render.

So as a know , defaultValue renders only for the first time. So if I set selected values to defaultValue attr it will not be rendered again after selection changes. value attr renders everytime so it will be updated after my new selection but it is becoming editable=false by the nature of the React.
I added onChange props to the component, it is not throwing any warning log also it is not reflecting by value change to the field.
So only solution is to render form on every onChange.

handleChange = (code,event)=> {
        console.log(event);
        var state = {};
        state[code] = event.target.value;
        this.setState(state);
};

Here is the ss for better explaining what I did.
screen shot 2016-02-04 at 15 56 26

@amorey
Copy link
Member

amorey commented Feb 4, 2016

Thanks for digging into this. I can take a closer look tonight. Which version of MUI are you using? I pushed some changes to the Input components in v0.4.1.

@serayuzgur
Copy link
Author

I am all up-to-date. If you provide your opinion also I can include it to my push request

@amorey
Copy link
Member

amorey commented Feb 4, 2016

What do you mean by becoming editable=false?

When I wrote the logic for the <Input> component I wanted to mimic the controlled/uncontrolled behavior of React <Input> components (https://facebook.github.io/react/docs/forms.html). The intended behavior is that you can control the value of MUI <Input> by calling .setState() in your own app/component. Here's a unit test that's designed to test for this condition:
https://github.com/muicss/mui/blob/master/test/react-tests/test-text-input.js#L82-L118

Are you having problems changing the value of the input element in the DOM by changing the value property of the MUI <Input> component in your render() method?

@serayuzgur
Copy link
Author

If I set value I can not edit it without providing onChange function and it is the expected result. In your test case you are setting the state of TestApp instance , it is the same thing that I did with Form. The thing is parentOfInputField.setState({...}) works but it means rendering the parent with all other children but inputField.setState({...}) is not working because input field's value is only taken from props not the state. What I say is, it should take initial value from state which copied value from props. Than any setState calls will affect it.
Like

 inputEl = (
        <input
          ref="inputEl"
          ...
          value={this.state.value}
          defaultValue={this.props.defaultValue}
          ...
        />

@amorey
Copy link
Member

amorey commented Feb 4, 2016

I see... is that the intended use of React components? I thought about doing that but I couldn't find any examples in the React documentation of an outer component calling an inner component's .setState() method. Do you know of any examples?

@serayuzgur
Copy link
Author

Summary: I think setState should update instance's value
As far as I know setState is for all modifications of component's state from anywhere. So in Component API under setState title it says event handlers. But in this case I gave the handler from parent.

Performs a shallow merge of nextState into current state. This is the primary method you use to trigger UI updates from event handlers and server request callbacks.

setState and correct usage of it is very important for performance so instead of rendering parent with other children react lets us to modify a child component. It has refs for reaching inner components.

@serayuzgur
Copy link
Author

And as an example I looked at Forms

handleChange: function(event) {
    this.setState({value: event.target.value});
  },
  render: function() {
    var value = this.state.value;
    return <input type="text" value={value} onChange={this.handleChange} />;
  }

As you see, value comes from state.

@amorey
Copy link
Member

amorey commented Feb 4, 2016

That example makes sense, but how is that different from this?

import React from 'react';
import TextInput from 'muicss/lib/react/text-input';

class Example extends React.Component {
  handleChange(ev) {
    this.setState({value: ev.target.value});
  }

  render() {
    return <TextInput value={this.state.value} onChange={this.handleChange.bind(this)} />;
  }
}

@amorey
Copy link
Member

amorey commented Feb 4, 2016

Can you show me an example of how you'd like to use <TextInput> so I can work backwards? What does your own component's code look like?

@serayuzgur
Copy link
Author

Hi , this is from your sample<TextInput value={this.state.value}
And this is from _input.js

  <input
          ref="inputEl"
          className={cls}
          type={this.props.type}
          value={this.props.value}

And I think it should be like

  <input
          ref="inputEl"
          className={cls}
          type={this.props.type}
          value={this.state.value}

@amorey
Copy link
Member

amorey commented Feb 4, 2016

It would still be useful to see how you expect it to work in your component. Does your use case look like this?

import React from 'react';
import TextInput from 'muicss/lib/react/text-input';

class Example extends React.Component {
  handleChange(ev) {
    this.setState({value: ev.target.value});
  }

  render() {
    return <TextInput value={this.state.value} onChange={this.handleChange.bind(this)} />;
  }
}

@amorey
Copy link
Member

amorey commented Feb 5, 2016

Or are you trying to do something like this?

import React from 'react';
import TextInput from 'muicss/lib/react/text-input';

class Example extends React.Component {
  changeValue(newVal) {
    this.refs.inputEl.setState({value: newVal});
  }

  render() {
    return <TextInput ref="inputEl" />;
  }
}

@serayuzgur
Copy link
Author

Sorry for delay, Timezone problems 😄 Here is my code

handleChange = (code, event)=> {
    this.refs[code].setState({
        value: event.target.value
    });
};
render(){
return <TextInput
    type="text"
    label="Name"
    floatingLabel={true}
    value="Seray"
    key="name"
    onChange={this.handleChange.bind(this,"name")}
    ref="name"
    defaultValue="Seray"
}

@amorey
Copy link
Member

amorey commented Feb 5, 2016

Thanks! That's very helpful. And just to make sure I understand - the reason you want to call .setState() on the <TextInput> instance is to improve performance?

@serayuzgur
Copy link
Author

Yes, I don't want to render whole form but just the related component on change. Also I think value is a state kind property. It can change internally, externally or via UI and this is what state is for. Also this will be important for other components to gain performance.

@amorey
Copy link
Member

amorey commented Feb 5, 2016

Ok thanks! Using this.state.value sounds like a good solution. I can take a closer look at this tonight.

@amorey
Copy link
Member

amorey commented Feb 6, 2016

I've been digging into this and I'm not sure if calling .setState() on the <TextInput> instance will improve performance.

I added some debugging statements to the constructor() and componentDidMount() methods in the <TextInput> component and I used this example to change the value property:

import React from 'react';
import Button from 'muicss/lib/react/button';
import TextInput from 'muicss/lib/react/text-input';

class Example extends React.Component {
  constructor(props) {
    super(props);

    this.state = {value: 'initial'};
  }

  onClick() {
    this.setState({value: Math.random().toString()});
  }

  render() {
    return (
      <div>
        <TextInput value={this.state.value} onChange={function(){}} />
        <Button onClick={this.onClick.bind(this)}>Update</Button>
      </div>
    );
  }
}

And what I found was that the component only gets mounted once and each subsequent button click will trigger a call to the render() method. What's the performance benefit of using setState() on the <TextInput> instance rather than triggering a change by using the value property?

@serayuzgur
Copy link
Author

Sorry for late response and thanks for not giving up 😄 . In your example, you only change TextInput value. By calling this.setState Entire Example renders again for a very changeable value.
Let's say you defined a ref="someInputRef" for TextInput and just call
this.refs.someInputRef.setState({value: Math.random().toString()}); at OnClick. It will only render the related field, in this case TextInput

Did I get it right?

@amorey
Copy link
Member

amorey commented Feb 11, 2016

I see... that makes sense.

Do you usually use this technique to update React <input> components? Have you seen any React documentation about pros/cons of updating a sub-component's state by using this.refs.someInputRef.setState()?

@serayuzgur
Copy link
Author

Not usually but, now I am writing a form renderer for datatable. It will be written once and must be well calculated. For docs you mentioned I am not sure but it seems Refs to Components is related. I think it supports my Idea.
The only think may be conflicting with my idea is the following quote but again I am not trying to "make things happen", just a single form which can set and retrieve form fields without rendering all.
Do you have any other docs. for me to look at?

If you have not programmed several apps with React, your first inclination is usually going to be to try to use refs to "make things happen" in your app. If this is the case, take a moment and think more critically about where state should be owned in the component hierarchy. Often, it becomes clear that the proper place to "own" that state is at a higher level in the hierarchy. Placing the state there often eliminates any desire to use refs to "make things happen" – instead, the data flow will usually accomplish your goal.

@amorey
Copy link
Member

amorey commented Feb 14, 2016

@serayuzgur Here's a new branch that supports the .setState() use case you're looking for:
https://github.com/muicss/mui/tree/input-setstate

After working on this, I'm not sure this is a use case we should support in an official way. Here's a quote from the Refs to Components document which says refs shouldn't be used as the go-to way to flow data:

Refs are a great way to send a message to a particular child instance in a way that would be inconvenient to do via streaming Reactive props and state. They should, however, not be your go-to abstraction for flowing data through your application. By default, use the Reactive data flow and save refs for use cases that are inherently non-reactive.

Do you have a sense for what the performance benefit is of calling .setState() instead of using props? Adding support for calling .setState() to the instance requires treating props in a special way in the wrapper components:
https://github.com/muicss/mui/blob/input-setstate/src/react/input.jsx#L38-L42

@serayuzgur
Copy link
Author

Hi @amorey
First of all thanks for the branch. I do have a sense for setState is faster than props but only if you structure it well. 😄
I want to ask you a question, here is the scenario
Let's say we have a form like

<Form>
  <Input ref="name" key="name" value="Name1"/>
  <Input ref="surname" key="surname" value="SurnameName1"/>
</Form>

How to would handle onChange?
I would define a function on the object and give them to the children like

render(){
  return (<Form>
    <Input ref="name" key="name" value="Name1" onChange={this.handleChange.bind(this,"name")}/>
    <Input ref="surname" key="surname" value="Surname1" onChange={this.handleChange.bind(this,"surname")}/>
  </Form>)
}
handleChange = (code, event)=> {
  this.refs[code].setState({
    innerValue: event.target.value
  });
};

Maybe my approach is wrong, but this confuses my mind.
Thinking in React

To make your UI interactive, you need to be able to trigger changes to your underlying data model. React makes this easy with state.

@amorey
Copy link
Member

amorey commented Feb 19, 2016

That should work except the mui:input-setstate branch uses the value property internally:

handleChange = (code, event) => {
  this.refs[code].setState({
    value: event.target.value
  });
};

What do you mean by confusing? Which part of the code do you want advice on?

I've been reading up on props vs state and I think modifying attributes like value via setState() is not recommended. React used to have a setProps() method which would have accomplished what you're trying to do but they recently deprecated it:
https://facebook.github.io/react/blog/2015/10/07/react-v0.14.html#new-deprecations-introduced-with-a-warning

Instead of setProps() React recommends calling ReactDOM.render() with the new props.

@serayuzgur
Copy link
Author

Thanks for all the help and explanation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants