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

Not triggering props.onChange initially and when props.value changes + Old props.onChange gets triggered #55

Open
vivekmoosani opened this issue Nov 23, 2016 · 9 comments

Comments

@vivekmoosani
Copy link

  1. Is it possible to not trigger the props.onChange() when Select2 is rendered the very first time or when props.value() is explicitly changed ?

  2. When I update both the props.value & props.onChange(), it calls the previously set change handler.
    This seems to be because the setValue() call in componentWillReceiveProps() triggers the change event and the componentWillUpdate() lifecycle method updates the handlers. Is there a particular reason why we update value in componentWillReceiveProps() ?
    https://github.com/rkit/react-select2-wrapper/blob/master/src/components/Select2.js#L54-L58

@rkit
Copy link
Owner

rkit commented Nov 24, 2016

  1. I think no.

Triggered whenever an option is selected or removed.
https://select2.github.io/options.html#events

  1. I will fix it soon

@rkit
Copy link
Owner

rkit commented Nov 24, 2016

Please, try a new pre-release 1.0.4-beta1

@vivekmoosani
Copy link
Author

Thanks Igor, I will test and update.

@vivekmoosani
Copy link
Author

vivekmoosani commented Nov 25, 2016

Hi Igor, Thanks the change works for my use case. Just a comment though. Should not it be 'prevProps' that are passed to this.detachEventHandlers() in updSelect2(). With the existing code you now cannot detach the handlers by simply not setting the corresponding props (onChange, onSelect ..)

this.detachEventHandlers(props);

Also on #1 from the issue,
I understand the trigger('change') call in updateValue() is needed for the select2 UI to update but this would also call the onChange() callback parent react component passes which seems unnecessary. We could add this check See master...vivekmoosani:master

@rkit
Copy link
Owner

rkit commented Nov 27, 2016

Should not it be 'prevProps' that are passed to this.detachEventHandlers() in updSelect2().

Oh, yes, you're right, but you mean nextProps instead of prevProps ?
We should detach the previous events.

Is it possible to not trigger the props.onChange() when Select2 is rendered the very first time or when props.value() is explicitly changed

… but this would also call the onChange() callback parent react component passes which seems unnecessary. We could add this check See

Yes, now I understand. I agree this needs to be fixed.

@vivekmoosani
Copy link
Author

Should not it be 'prevProps' that are passed to this.detachEventHandlers() in updSelect2().

Oh, yes, you're right, but you mean nextProps instead of prevProps ?We should detach the previous events.

I meant prevProps. Say for the first render you passed
<Select2 onChange={change1} onSelect={select1} />
but then then you update props so that you only pass onChange
<Select2 onChange={change2} />

updSelect2(props) - if you do this.detachEventHandlers(props); where props are the nextProps from componentWillReceiveProps() it would detach change1 but not select1 as props.onSelect is undefined. So we should do this.detachEventHandlers(prevProps)

@rkit
Copy link
Owner

rkit commented Nov 28, 2016

Yes, I meant the same. Should be prevProps.

We should detach the previous events.

I will fix it soon. Thanks for the report.

rkit added a commit that referenced this issue Nov 30, 2016
@rkit
Copy link
Owner

rkit commented Nov 30, 2016

Please, try a new pre-release 1.0.4-beta2

@bovan
Copy link

bovan commented Jan 30, 2017

@rkit seems to work here,
edit: seems to get callstack error though, investigating.. get change events in loop when I try to load the values (I use https://github.com/davidkpiano/react-redux-form )

edit2: seemed to be the regular dropdown in same form as my multiselect which went on a loop when new props triggered change event. I changed dropdown to using onSelect and multiselect to use onChange which seems to work. The const handlerChanged = e => prevProps[e[1]] !== props[e[1]]; check was always true for the dropdown when using onChange

unrelated note: onUnSelect seems to trigger react unknown prop warning

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

3 participants