-
Notifications
You must be signed in to change notification settings - Fork 202
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
Re rendering DOM node with updated state data #216
Re rendering DOM node with updated state data #216
Conversation
Context: `this.$picker.daterangepicker` in ComponentDidMount was creating calendar on Body element and `bootstrap-daterangepicker` dependency does not update DOM implicity based on state data changes like ranges and other. (good OLD JQuery days :D). This was causing the behaviour where other than start and end date, all other props where not updating the calendar. Fix: This PR is making sure that we will re-render DOM node associated with calendar again, once we witness state data changes (except start and end date) through `setOptionsFromProps`.
Thanks for this PR! I will try to pull down your branch and do some manual testing in the next few days. Seems like the CI is failing due to a node6 issue that probably pre-existed. I'll try to take a look at that as well |
+1 for prompt reply, I will let you know shortly after writing unit tests as well as some more fixes for this. I realized, event binding issue still exists in this PR. |
Hello, is the above PR merged ? I am facing the same issue wherein the calendar is not re-re dering on a change in the ranges prop. |
@rehanuma / @vidhi6891 - Sorry- I forgot about this. I just tested and merged. I'll publish a new version shortly. |
I encountered a bunch of issues while trying to publish b/c I had to fix a bunch of things in the master branch. Anyways, I just published v5.0.0 with this change: https://projects.skratchdot.com/react-bootstrap-daterangepicker/ https://www.npmjs.com/package/react-bootstrap-daterangepicker |
@skratchdot and @rehanumar , thanks for your actions! Once I add the I'm also getting the following warning in the console, could this have something to do with it?
This is how my component looks
Hereby the versions of the packages I'm using
|
@RonHouben - weird. here's an example: I notice that the "story" above isn't using Seems like there are still some bugs. It would help to have a working example of the bug on codesandox.io or something. Guessing it's a combination of the props you are using. I'd be interested in removing these props 1 by 1 and seeing if/when things work as expected:
|
I see someone logged #223 for the issue @RonHouben mentioned above (in #216 (comment)) |
I'm able to reproduce this: Assuming this line:
removes all event handlers (and doesn't work with non-simple data. I should've had a better code review |
I think it was added because the "currentOptions" were stale, so adding that line fixed a few issues, but broke others. I don't fully understand the issue @rehanumar was trying to fix |
Actually- I see the issue. When I get some time, I'm just going to change the API of this project. People are trying to use it as a "controlled component", but it doesn't work that way. I'm going to make all the "options" settable via: "initialOptions". Then I'm going to expose the 6 events the upstream lib supports. The only 2 "controlled" options will be startDate and endDate (because that's all the upstream lib supports). |
@rehanumar - i had to remove you commit b/c it broke other things. my suggestion: any prop besides startDate or endDate that you need to "change"- then you need to render a new instance of "DateRangePicker". I changed the story you added to reflect how to accomplish "change the range": react-bootstrap-daterangepicker/stories/index.js Lines 81 to 152 in 22517d7
|
FYI- I just published a new version v6.0.0. The examples have been updated: |
Context:
this.$picker.daterangepicker
in ComponentDidMount was creating calendar on Body element andbootstrap-daterangepicker
dependency does not update DOM implicity based on state data changes like ranges and other. (good OLD JQuery days :D). This was causing the behaviour where other than start and end date, all other props where not updating the calendar.Fix: This PR is making sure that we will re-render DOM node associated with calendar again, once we witness state data changes (except start and end date) through
setOptionsFromProps
.