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

Add: value property #16

Merged
merged 1 commit into from
Feb 5, 2016
Merged

Add: value property #16

merged 1 commit into from
Feb 5, 2016

Conversation

lexich
Copy link
Contributor

@lexich lexich commented Jan 28, 2016

It's necessary when we value was changed from external.

<Select2 value={ .... } />

@rkit
Copy link
Owner

rkit commented Jan 28, 2016

I am getting error:

Warning: Failed form propType: You provided a `value` prop to a form field without an `onChange`  
handler. This will render a read-only field. If the field should be mutable use `defaultValue`.   
Otherwise,   set either `onChange` or `readOnly`. Check the render method of `Select2`.

This solution is not suitable?

this.refs.tags.el.val('value').trigger('change');

@lexich
Copy link
Contributor Author

lexich commented Jan 28, 2016

You solution, unfortunately does't work (I had errors after uglify code and also receve internal select2 error). But after this patch - all right. And I'll try to fix warning

@@ -53,7 +69,7 @@ export default class Select2 extends Component {
}

render() {
const { data, ...params } = this.props;
const { data, value, ...params } = this.props;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Problem with React warning was here, because select receive property value, but we not need it

@lexich
Copy link
Contributor Author

lexich commented Feb 1, 2016

I think feature is ready. What is your opinion? @rkit

@rkit
Copy link
Owner

rkit commented Feb 1, 2016

Sorry for the long answer… Now I am checking this feature.

@rkit
Copy link
Owner

rkit commented Feb 1, 2016

  1. In this code the defaultValue does not work
  2. Please fix code style (see .eslintrc) in Tags.js and Select2.js

@lexich lexich force-pushed the add_value_property branch 2 times, most recently from 779b49d to 3a424f7 Compare February 1, 2016 16:22
@lexich
Copy link
Contributor Author

lexich commented Feb 1, 2016

@rkit fix code style and bug with code

@rkit
Copy link
Owner

rkit commented Feb 1, 2016

Thanks! Tomorrow will be release.

@lexich
Copy link
Contributor Author

lexich commented Feb 1, 2016

👍

@rkit
Copy link
Owner

rkit commented Feb 2, 2016

It seems there is a small discrepancy in logic:

if multiple as true, and set properties defaultValue and value, defaultValue not replaced on value.
But if multiple as false, defaultValue replaced on value

@lexich
Copy link
Contributor Author

lexich commented Feb 2, 2016

multiple: true and defaultValue isn't compatible options.

multiple as false, defaultValue not replaced on value => it's only default init by value. It may be replace with setValue in componentDidMount.

Anyway, defaultValue it's very strange prop.

@lexich lexich mentioned this pull request Feb 2, 2016
2 tasks
@rkit
Copy link
Owner

rkit commented Feb 2, 2016

  1. multiple: true and defaultValue isn't compatible options

    Maybe so?

    componentDidMount() {
      
     const { value } = this.props;
     if (value !== undefined) {
       this.setValue(value);
     }
    
  2. And what for is this needed?

    // in render()
    if ((value !== (void 0) && value !== null)) {
      if (!params.multiple) {
        params.defaultValue = value;
      }
    }

@lexich
Copy link
Contributor Author

lexich commented Feb 2, 2016

@rkit You are right, my solution was overcomplicated. I fix it

@rkit
Copy link
Owner

rkit commented Feb 4, 2016

@lexich You still in process?

I still not sure about this:

if (!defaultValue && value !== (void 0)) {
  this.setValue(value);
}

if defaultValue would be 0, the value will be taken from value, otherwise from defaultValue.

@@ -45,6 +51,16 @@ export default class Select2 extends Component {
this.props.events.forEach(event => {
this.el.on(event[0], this.props[event[1]]);
});
const { defaultValue, value } = this.props;
if (defaultValue === (void 0) && value !== (void 0)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rkit Fix: setValue if only defaultValue isn't defined

@rkit
Copy link
Owner

rkit commented Feb 5, 2016

Thanks!

rkit added a commit that referenced this pull request Feb 5, 2016
@rkit rkit merged commit ac5d610 into rkit:master Feb 5, 2016
@rkit
Copy link
Owner

rkit commented Feb 5, 2016

@lexich I found a few problems with shallowEqual:

  1. this.el.val() — always returning string or string[], accordingly if pass values as in numerics, the shallowEqual will return false
  2. Also this.el.val() may return the values in the array in a different order, e.g. http://jmp.sh/pzIqGGI

@lexich lexich deleted the add_value_property branch February 5, 2016 07:33
@lexich
Copy link
Contributor Author

lexich commented Feb 5, 2016

i wrote custom implementation for our case #19

@rkit
Copy link
Owner

rkit commented Feb 6, 2016

This is a good solution, thanks.
But I still do not understand, in your case really need to use shallowEqual ?
Because, the component will be updated anyway… What will be the real benefit?

@lexich
Copy link
Contributor Author

lexich commented Feb 8, 2016

export default class Tags extends Component {
  constructor(props) {
    super(props);
    this.state = { value1: ['feature'] };
  }
  render() {
    const { value } = this.state;
    return (
        <Select2
          data={['bug', 'feature', 'documents', 'discussion']}
          value={ value1 }
          onChange={(e)=> {
            this.setState({ value1: $(e.target).val() });
          }}
        />
    );
  }
}

We have value1 === "feature". componentDidMount hook invoke setValue. setValue triggered onChange hook. Our implementation (I think it's the most common implementation) call setState of value1.
($(e.target).val() is ["feature"]) !== (value1 is ["feature"]). componentWillReceiveProps hook invoke setValue once again. We have infinite loop.

Also shallowEqual need for #18

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

Successfully merging this pull request may close these issues.

None yet

3 participants