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

Re rendering DOM node with updated state data #216

Conversation

rehanumar
Copy link
Contributor

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.

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`.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling d8befdc on rehanumar:bugfix/rehan-update_UI_with_latest_data into 47755d5 on skratchdot:master.

@skratchdot
Copy link
Owner

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

@rehanumar
Copy link
Contributor Author

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.

@vidhi6891
Copy link

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.

@skratchdot skratchdot merged commit 7875060 into skratchdot:master Jul 16, 2020
@skratchdot
Copy link
Owner

@rehanuma / @vidhi6891 - Sorry- I forgot about this. I just tested and merged. I'll publish a new version shortly.

@skratchdot
Copy link
Owner

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

@RonHouben
Copy link

RonHouben commented Jul 23, 2020

@skratchdot and @rehanumar , thanks for your actions!
Unfortunately it's still not working with v5.0.0.

Once I add the showCustomRangeLabel and ranges props, the onApply event doesn't trigger anymore.

I'm also getting the following warning in the console, could this have something to do with it?

index.js:1 Warning: Using UNSAFE_componentWillReceiveProps in strict mode is not recommended and may indicate bugs in your code. See https://fb.me/react-unsafe-component-lifecycles for details.

* Move data fetching code or side effects to componentDidUpdate.
* If you're updating state whenever props change, refactor your code to use memoization techniques or move it to static getDerivedStateFromProps. Learn more at: https://fb.me/react-derived-state

Please update the following components: DateRangePicker

This is how my component looks

<DateRangePicker
            startDate={selectedStartDate}
            endDate={selectedEndDate}
            onApply={handleDateChange}
            autoApply
            showCustomRangeLabel
            alwaysShowCalendars={true}
            ranges={{
              range1: [
                moment().subtract(1, 'month').toDate(),
                moment().add(1, 'month').toDate(),
              ],
              range2: [
                moment().subtract(1, 'year').toDate(),
                moment().add(1, 'year').toDate(),
              ],
            }}
          >
            <button>Click Me To Open Picker!</button
</DateRangePicker>

Hereby the versions of the packages I'm using

    "bootstrap": "^4.5.0",
    "bootstrap-daterangepicker": "^3.1.0",
    "jquery": "^3.5.1",
    "moment": "^2.27.0",
    "prop-types": "^15.7.2",
    "react-bootstrap-daterangepicker": "^5.0.0",

@skratchdot
Copy link
Owner

@RonHouben - weird. here's an example:
https://projects.skratchdot.com/react-bootstrap-daterangepicker/?path=/story/daterangepicker--propsupdate
and the code for that story:
https://github.com/skratchdot/react-bootstrap-daterangepicker/blob/master/stories/index.js#L112-L123

I notice that the "story" above isn't using showCustomRangeLabel.

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:

            autoApply
            showCustomRangeLabel
            alwaysShowCalendars={true}

@skratchdot
Copy link
Owner

skratchdot commented Jul 23, 2020

I see someone logged #223 for the issue @RonHouben mentioned above (in #216 (comment))

@skratchdot
Copy link
Owner

skratchdot commented Jul 23, 2020

I'm able to reproduce this:
https://codesandbox.io/s/gifted-mountain-d63xq?file=/src/App.js

Assuming this line:

this.$picker.daterangepicker(this.$picker.data('daterangepicker'));

removes all event handlers (and doesn't work with non-simple data. I should've had a better code review

@skratchdot
Copy link
Owner

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

@skratchdot
Copy link
Owner

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).

@skratchdot
Copy link
Owner

@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":

.add('Change range data', () => {
const StoryComp = () => {
const openButtonLabel = text(
'open button label',
'click to open date range picker'
);
const changeButtonLabel = text(
'change button label',
'change range label'
);
const keyRef = useRef(Date.now());
const [dates, setDates] = useState({
startDate: moment('2020/03/01'),
endDate: moment('2020/03/15'),
});
const [ranges, setRanges] = useState({
['First Range']: [
moment().subtract(2, 'days'),
moment().add(2, 'days'),
],
});
const handleApply = (event, picker) => {
setDates({
startDate: picker.startDate,
endDate: picker.endDate,
});
};
const randomNumber = () => Math.floor(Math.random() * 20) + 1;
const handleChangeRanges = () => {
keyRef.current = Date.now();
setRanges({
[`Range ${Date.now()}`]: [
moment().subtract(randomNumber(), 'days').startOf('day'),
moment().add(randomNumber(), 'days').startOf('day'),
],
});
};
return (
<div>
<DateRangePicker
key={keyRef.current}
onApply={handleApply}
onCancel={action('onCancel')}
onEvent={action('onEvent')}
onHide={action('onHide')}
onHideCalendar={action('onHideCalendar')}
onShow={action('onShow')}
onShowCalendar={action('onShowCalendar')}
ranges={ranges}
>
<button>{openButtonLabel}</button>
</DateRangePicker>
<br />
<h4>
startDate: <small>{dates.startDate.format()}</small>
</h4>
<h4>
endDate: <small>{dates.endDate.format()}</small>
</h4>
<h4>
ranges: <small>{JSON.stringify(ranges)}</small>
</h4>
<button onClick={handleChangeRanges}>{changeButtonLabel}</button>
</div>
);
};
return (
<div>
<StoryComp />
</div>
);
});

@skratchdot
Copy link
Owner

FYI-

I just published a new version v6.0.0. The examples have been updated:
https://projects.skratchdot.com/react-bootstrap-daterangepicker/

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

5 participants