longbill / jquery-date-range-picker

A jQuery plugin that allows user to select a date range
MIT License
1.12k stars 579 forks source link

Month UNDEFINED if shortcuts and monthSelect are enabled #412

Closed XavierCLL closed 5 years ago

XavierCLL commented 6 years ago

The month showed as UNDEFINED if shortcuts and monthSelect are enabled: screenshot_20180313_155839

Config:

{
       shortcuts :
            {
                'prev-days': [1,3,5,7],
                'prev': ['week','month','year'],
                'next-days':null,
                'next':null
            },
        monthSelect: true,
    yearSelect: true,
}

Using jquery-date-range-picker v 0.16.1

kl23 commented 6 years ago

I changed a little bit as a workaround, there should be a mutator that update both of the 2 months instead of just modifying this function because it updates without this function in elsewhere. And some of the updates rendered the calendar more than once.

function redrawDatePicker() {
    var _endDate = new Date(opt.endDate.valueOf());
    _endDate.setHours(0,0,0,0);
    _endDate.setDate(0);
    _endDate.setMilliseconds(-1);
    if (opt.month1 > _endDate) {
        showMonth(_endDate, 'month1');
        showMonth(nextMonth(_endDate), 'month2');
    } else {
        showMonth(opt.month1, 'month1');
        showMonth(opt.month2, 'month2');
    }
}
XavierCLL commented 6 years ago

Hi @kl23 , I tried with your changes and the issue is still present, I forgot to say that the error is showing when you pick the shortcuts.

kl23 commented 6 years ago

Thanks for your reply, @XavierCLL . I in fact encountered the same problem by clicking a date range in the same month and the range is in the maximum month (i.e. endDate). As mentioned in the above comment, it updates (renders UI) without this function in elsewhere (not just redrawDatePicker).

The UNDEFINED string is generated here since isCurrent is never true: https://github.com/longbill/jquery-date-range-picker/blob/23cbf1fca097a9665e310a1054081f5cb8e8bd72/src/jquery.daterangepicker.js#L2018-L2028 and it loops through the input from range created in generateMonthElement() (line 2003). The range depends on the first parameter in showMonth() and is limited by the maximum date that you can choose, thereby the list will not include Apr in case max date is Mar. But sorry, I don't understand how range is generated, you may take a look here: https://github.com/longbill/jquery-date-range-picker/blob/23cbf1fca097a9665e310a1054081f5cb8e8bd72/src/jquery.daterangepicker.js#L1945-L1963

The problem is: showMonth() is used everywhere in the source code. And it fails to create a correct range when the selected start date is in the same month of endDate.

The code changed in above comment shifted the pair of months to left (month -= 1) so that (1) the selected date range are on the right, (2) maximum date is either on the right or in the next page of right month. You may try to apply this logic to where it triggers showMonth().

wakirin commented 6 years ago

https://jsfiddle.net/63ks4vbo/3/

I don't see that problem, can you show your trouble on jsfiddle ?

yphoenix commented 6 years ago

I can't see an issue either. I updated the fiddle to include a moment.js library. I did purposely include v2.9 as the docs state that we only need 2.8.1, whereas in reality we need to either fix #421 or update the docs to require 2.12.

https://jsfiddle.net/63ks4vbo/7/

wakirin commented 6 years ago

ok, thanks. I guess better update docs.

XavierCLL commented 6 years ago

Hi @wakirin and @yphoenix you are right I forget the jsfiddle for to show the error, please go to:

https://jsfiddle.net/tuteq2ae/1/

And click on a shortcut 1day or 3days

XavierCLL commented 6 years ago

maybe this bug is related with moment() library because if you delete the line of endDate the error disappears, the #421 fix that?

wakirin commented 6 years ago

Found problem in function generateMonthElement https://github.com/longbill/jquery-date-range-picker/blob/766ea70b9f1ac2d67ddc4c53955f858bf6600b3e/src/jquery.daterangepicker.js#L1958

I have replaced endDate.get('month') to date.get('month'), and now it's show month names.

jsfiddle with updated jquery.daterangepicker.js https://jsfiddle.net/tuteq2ae/5/

XavierCLL commented 6 years ago

Yeah @wakirin with your changes this issue is fixed, thank you! please do the PR

holtkamp commented 6 years ago

Released in https://github.com/longbill/jquery-date-range-picker/releases/tag/0.16.3

monovertex commented 5 years ago

I'm re-opening this issue, because the fix presented here breaks the month dropdown. See #452.

The issue is a bit more involved. The problem is that the select builder depends on the range constructed in generateMonthElement to extract the current month, but it should be completely independent instead. So the initial range building was good, it's just the select builder that is broken.

I will open a PR with a more robust fix soon.

monovertex commented 5 years ago

@XavierCLL , please see #455 and let me know if it fixes your issue. You can test the fixed version in this JSFiddle.

XavierCLL commented 5 years ago

Hi @monovertex, for me works perfect, I tested here: https://jsfiddle.net/xwb62hdn/9/ thanks!

monovertex commented 5 years ago

@XavierCLL, unfortunately there was still one bug leftover. Please see this new jsfiddle. It uses the same config as in your jsfiddle, and it seems to be okay. Thanks for the help!