Bug/range not updated

[aommm]

This fixes #5.

This pull request listens for prop changes, and whenever any of those two properties change, we re-parse them.

Note/discussion:
this might be a problem for other properties as well, but I only solved it for my use case for now.
Ideally, we would perhaps restructure the code so that the dates information is not stored in two places (props and state). One could for instance store the dates only as props, and do the parsing of the dates only when needed (i.e. in render)

[burakcan]

Thank you @aommm :)

i think it may be better if you use setRange method of the DateRange component instead setting the state directly while receiving new props.
https://github.com/aommm/react-date-range/blob/bug/range-not-updated/src/DateRange.js#L44

[aommm]

Good point, have done so now!

[aommm]

Whoops. Noticed an error (in my code) when testing the latest commit.

setRange also calls the onChange listener, and in my case, that listener updates the x and y properties. This in turn triggers a new setRange, and so on into infinity.

Toy example of my setup:

render() {
  return <DateRange startDate={x} endDate={y} onChange={this.datepickerChanged.bind(this)} />
}
...
datepickerChanged(range) {
  this.setState({x: range.startDate, y: range.endDate});
}

If you have this setup and trigger a change of x or y, you end up in an infinite loop.
I think this is a fairly common use case, so it would be nice if it could be supported.

Proposal:
onChange should only trigger when dates change 'internally', that is, when the user changes them from within the datepicker component.
This could be achieved by adding options to the setRange method, so I can pass {silent: true} in my use case above. If silent is true, it would not trigger any onChange-event.

What do you think about this?

[burakcan]

@aommm good point. sure we need a way to do silent changes and having { silent : true } looks good to me.

[burakcan]

@aommm is this ready to be merged?

[aommm]

Yes, I've tested it and it behaves as expected.
Den 17 okt 2015 18:55 skrev "Burak Can" notifications@github.com:

@aommm https://github.com/aommm is this ready to be merged?

—
Reply to this email directly or view it on GitHub
#6 (comment)
.

[burakcan]

thanks @aommm

[burakcan]

@aommm i'm reverting this back since it's breaking current behavior which can be reproduced in the demo app. If you run the demo app with your build and try to change the range manually (by clicking) you can see what's broken.