gcanti / tcomb-form

Forms library for react
https://gcanti.github.io/tcomb-form
MIT License
1.16k stars 136 forks source link

Update options with onChange event #169

Closed johnraz closed 8 years ago

johnraz commented 9 years ago

Hi again,

I've been following the example here to disable a field based on another's value.

First, I cannot seem to make it work, the state is correctly updated (fetching it the next time I render the component shows that the value has been updated correctly), but, the change is not reflected in the rendered form.

Second, how could I update only one field of one item of a list (list of struct) ? With the above technique, the change would target the field for all the items of my list, in my use case I only want to update 1 field of 1 specific item.

to make it clear, I have a model that looks like this:

var CriteriaPhoneNumbers = t.struct({
    allNumbers: t.Bool,
    phoneNumbers: t.list(t.Str)
});

var CriteriaOptions = t.struct({
    active: t.Bool,
    schedule: t.maybe(schedules),
    phoneNumberTargets: CriteriaPhoneNumbers
});

var GlobalSelectiveOptions = t.struct({
    active: t.Bool,
    forwardTo: t.Str,
    splashRing: t.Bool,
    criteria: t.list(CriteriaOptions)
});

I need to disable phoneNumbers depending on the value of allNumbers per instance of the CriteriaPhoneNumbers struct.

johnraz commented 9 years ago

Ok, I just found the answer to the first part of my question... I was not using this.state.options to assign the initial options props on my form ... I was using the options variable instead ... So you can ignore the first part of my question, the second one is still valid though ;-)

gcanti commented 9 years ago

how could I update only one field of one item of a list (list of struct) ?

Hi @johnraz, I'm afraid it's not possible with the current API.

I could try to find a workaround. If I understand, with the CriteriaPhoneNumbers struct, you are implementing a sort of union: either you turn on the allNumbers flag or enter a list of numbers, right?

johnraz commented 9 years ago

Yep, it's exactly that.

I was thinking of building a custom component that would be bound to the struct and have the proper behavior but I have no idea if its doable. I'm currently digging the source, let me know if you think of something.

gcanti commented 9 years ago

I was thinking of building a custom component

Me too, this is my first attempt. It's kinda hacky and possibly expensive (many re-renderings) but seems to work (sorry for the ES6/ES7 syntax):

import React from 'react';
import t from 'tcomb-form';

// custom component
class CriteriaPhoneNumbersStructComponent extends t.form.Struct {

  onChange(...args) {
    super.onChange(...args);
    // change the options on the fly based on allNumbers value
    this.props.options.fields.phoneNumbers = {
      ...this.props.options.fields.phoneNumbers,
      disabled: this.state.value.allNumbers // enable / disable all the list fieldset
    };
    this.forceUpdate(); // fool the default shouldComponentUpdate
  }

}

var CriteriaPhoneNumbers = t.struct({
    allNumbers: t.Bool,
    phoneNumbers: t.list(t.Str)
});

// ignore CriteriaOptions and GlobalSelectiveOptions for simplicity...
var Type = t.list(CriteriaPhoneNumbers);

const options = {
  disableOrder: true, // reduce noise in the UI
  item: {
    factory: CriteriaPhoneNumbersStructComponent,
    fields: {
      phoneNumbers: {
        disableOrder: true // reduce noise in the UI
      }
    }
  }
};

const App = React.createClass({

  onSubmit(evt) {
    evt.preventDefault();
    const value = this.refs.form.getValue();
    if (value) {
      console.log(value);
    }
  },

  render() {
    return (
      <form onSubmit={this.onSubmit}>
        <t.form.Form
          ref="form"
          type={Type}
          options={options}
        />
        <button className="btn btn-primary">Save</button>
      </form>
    );
  }

});
johnraz commented 9 years ago

Babel doesn't seem to like this ...this.props.options.fields.phoneNumbers,

    this.props.options.fields.phoneNumbers = {
      ...this.props.options.fields.phoneNumbers,
      disabled: this.state.value.allNumbers // enable / disable all the list fieldset
    };

Edit: removing the line seems to work though.

gcanti commented 9 years ago

It's a ES7 feature, enabled with --stage=1 (https://github.com/sebmarkbage/ecmascript-rest-spread).

same as

import assign from 'object-assign'; // ES6 Object.assign() polyfill

// custom component
class CriteriaPhoneNumbersStructComponent extends t.form.Struct {

  onChange(...args) {
    super.onChange(...args);
    // change the options on the fly based on allNumbers value
    this.props.options.fields.phoneNumbers = assign(
      {}, 
      this.props.options.fields.phoneNumbers, 
      {disabled: this.state.value.allNumbers}
    );
    this.forceUpdate(); // fool the default shouldComponentUpdate
  }

}

in ES6

johnraz commented 9 years ago

Awesome, enabling stage=1 made it work as expected \o/.

Now, what are our options to avoid the forceUpdate ? Can you give me a bit more insight on why it is required ?

edit: I thought changing this.props would trigger a re-render ?

gcanti commented 9 years ago

Can you give me a bit more insight on why it is required ?

Because of the shouldComponentUpdate implementation

https://github.com/gcanti/tcomb-form/blob/master/src/components.js#L129

Since the reference to this.props.options doesn't change you must force an update

thought changing this.props would trigger a re-render ?

You can't, props are read-only for React (it will issue a warning otherwise)

johnraz commented 9 years ago

I had to do the following for this solution to work on initial load (before a change occurs):

class CriteriaPhoneNumbersStructComponent extends t.form.Struct {
    getLocals(){
        let locals = super.getLocals();
        console.log('pre:', locals);
        console.log(locals.value);
        locals.inputs.phoneNumbers.props.options.disabled = (locals.value.hasOwnProperty('allNumbers') && locals.value.allNumbers);

        console.log('after:', locals);
        return locals;

    }
    onChange(...args) {
        super.onChange(...args);

        // change the options on the fly based on allNumbers value
        this.props.options.fields.phoneNumbers = Object.assign(
            {},
            this.props.options.fields.phoneNumbers,
            {disabled: this.state.value.allNumbers}
        );

        this.forceUpdate(); // fool the default shouldComponentUpdate
    }
}

Is it acceptable to change the propsthere ? and if not what would be the right way?

And also by looking at it, can't we achieve something by overriding the getInputs method ?

gcanti commented 9 years ago

work on initial load (before a change occurs):

And also by looking at it, can't we achieve something by overriding the getInputs method ?

Oh you are right! I forgot that. What about?

// custom component
class CriteriaPhoneNumbersStructComponent extends t.form.Struct {

  onChange(...args) {
    super.onChange(...args);
    this.forceUpdate(); // fool the default shouldComponentUpdate
  }

  getInputs() {
    // change the options on the fly based on allNumbers value
    this.props.options.fields.phoneNumbers = {
      ...this.props.options.fields.phoneNumbers,
      disabled: this.state.value.allNumbers // enable / disable all the list fieldset
    };
    return super.getInputs();
  }

}
johnraz commented 9 years ago

Yep it does indeed work and solve both cases (initial load and change), hooray \o/. Is the use of forceUpdate a big deal ?

Still, having one part of a form enabled/disabled by another even in a list, looks like a quite common pattern, it would be nice to find the "right" way to do it.

I still need a bit more time to make myself at ease with both the concept and source of tcomb-formand React itself to be fully capable of thinking of a potential clean solution :-)

Thanks for the valuable informations again by the way.

gcanti commented 9 years ago

Is the use of forceUpdate a big deal ?

Maybe not. A slightly better version:

class CriteriaPhoneNumbersStructComponent extends t.form.Struct {

  onChange(fieldName, fieldValue, path, kind) {
    super.onChange(fieldName, fieldValue, path, kind);
    if (fieldName === 'allNumbers') { // reduce useless re-renderings
      this.forceUpdate();
    }
  }

  getInputs() {
    this.props.options.fields.phoneNumbers = {
      ...this.props.options.fields.phoneNumbers,
      disabled: this.state.value.allNumbers
    };
    return super.getInputs();
  }

}

Still, having one part of a form enabled/disabled by another even in a list, looks like a quite common pattern, it would be nice to find the "right" way to do it.

I agree. My only concern is: there are so many use cases out there that maybe it's better solving them in a custom way as we did without burdening the API

johnraz commented 9 years ago

Still, having one part of a form enabled/disabled by another even in a list, looks like a quite common pattern, it would be nice to find the "right" way to do it.

I agree. My only concern is: there are so many use cases out there that maybe it's better solving them in a custom way as we done without burdening the API

I totally agree that keeping the API small will help keep the library stable and efficient. Again, maybe am I totally off base, but I was more thinking of some kind of hook / callback that would trigger before each component of a list item is created / rerendered.

It shouldn't be too much of a burden, but again, I'm speaking quite out of knowledge :-)

pbreah commented 9 years ago

gcanti, first congrats for this very useful library.

Just a suggestion, as I saw this interesting discussion:

Wouldn't the form be easily customized on the fly, if the form structure itself (and options) was part of the state? then when any changes are made on the client code (that affects the form structure "state"), would re-render the form controls?

This with a factory utility method that can translate json into tcomb, could even allow to pull form layouts and configurations from the server without using dangerous conversions from string to JS.

What do you think? ...just wanted to share this idea, as I think you have a great library.

gcanti commented 9 years ago

Hi @pbreah, Thanks!

Your suggestions are great. Actually the first one is the suggested way for dealing with dynamic forms, see https://github.com/gcanti/tcomb-form/blob/master/GUIDE.md#rendering-options (Example: disable a field based on another field's value)

You could even store the type prop in the state... There are some examples in the issues searching for "dynamic forms":

https://github.com/gcanti/tcomb-form/issues?utf8=✓&q=label%3A%22dynamic+forms%22+

Thanks for sharing your ideas, Giulio

Liooo commented 5 years ago

careful, changing any attribute on options won't update the component, because internally implemented shouldComponentUpdate will check the props change by reference.

rough sketch here https://codesandbox.io/s/rw9r470ryn