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

Month UNDEFINED if shortcuts and monthSelect are enabled #412

Closed
XavierCLL opened this issue Mar 13, 2018 · 15 comments · Fixed by #425 or #455
Closed

Month UNDEFINED if shortcuts and monthSelect are enabled #412

XavierCLL opened this issue Mar 13, 2018 · 15 comments · Fixed by #425 or #455
Assignees
Labels
Status: done Type: regression Valid bug in a previously working feature, confirmed by a contributor.

Comments

@XavierCLL
Copy link

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
Copy link

kl23 commented Mar 15, 2018

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
Copy link
Author

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
Copy link

kl23 commented Mar 15, 2018

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:

for (var i = 0, l = data.length; i < l; i++) {
select += '<option value="' + data[i].value + '"' + (data[i].isCurrent ? ' selected' : '') + '>';
select += data[i].text;
select += '</option>';
if (data[i].isCurrent) {
current = data[i].text;
}
}
select += '</select>' + current + '</div>';

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:
function generateMonthElement(date, month) {
var range;
var startDate = opt.startDate ? moment(opt.startDate).add(!opt.singleMonth && month === 'month2' ? 1 : 0, 'month') : false;
var endDate = opt.endDate ? moment(opt.endDate).add(!opt.singleMonth && month === 'month1' ? -1 : 0, 'month') : false;
date = moment(date);
if (!opt.monthSelect ||
startDate && endDate && startDate.isSame(endDate, 'month')) {
return '<div class="month-element">' + nameMonth(date.get('month')) + '</div>';
}
range = [
startDate && date.isSame(startDate, 'year') ? startDate.get('month') : 0,
endDate && date.isSame(endDate, 'year') ? endDate.get('month') : 11
];
if (range[0] === range[1]) {
return '<div class="month-element">' + nameMonth(date.get('month')) + '</div>';
}

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
Copy link
Contributor

wakirin commented Apr 5, 2018

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

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

@yphoenix
Copy link

yphoenix commented Apr 5, 2018

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
Copy link
Contributor

wakirin commented Apr 5, 2018

ok, thanks. I guess better update docs.

@XavierCLL
Copy link
Author

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
Copy link
Author

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

@wakirin
Copy link
Contributor

wakirin commented Apr 5, 2018

Found problem in function generateMonthElement

endDate && date.isSame(endDate, 'year') ? endDate.get('month') : 11

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
Copy link
Author

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

@holtkamp
Copy link
Collaborator

holtkamp commented Apr 5, 2018

@monovertex
Copy link
Collaborator

monovertex commented Sep 23, 2018

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 monovertex reopened this Sep 23, 2018
@monovertex monovertex added Type: regression Valid bug in a previously working feature, confirmed by a contributor. Status: todo Accepted issue that needs someone to fix it. labels Sep 23, 2018
@monovertex monovertex self-assigned this Sep 23, 2018
@monovertex
Copy link
Collaborator

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

@XavierCLL
Copy link
Author

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

@monovertex
Copy link
Collaborator

@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!

monovertex added a commit that referenced this issue Sep 26, 2018
…ent-month-when-generating-month-select

Bugfix/#412, #452 - Fix out-of-bounds options when building the month/year selects
@monovertex monovertex added Status: done and removed Status: todo Accepted issue that needs someone to fix it. labels Dec 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: done Type: regression Valid bug in a previously working feature, confirmed by a contributor.
Projects
None yet
6 participants