search

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

Fix changes to month dropdown #421

Open yphoenix opened 6 years ago

yphoenix commented 6 years ago

When using the month dropdown option changes don’t take effect because moment(opt[type])name is passed a string. The momentJS docs state that this works when name===‘month’ but testing reveals that this isn’t the case. Always passing a Number instead of a String always works.

holtkamp commented 6 years ago

Nice find! Thanks, can you also provide a example of this problem using https://jsfiddle.net?

yphoenix commented 6 years ago

Only happens with older versions of the MomentJS Library. Turns out it is fixed in 2.12 (https://github.com/moment/moment/pull/2897)

Anyway, here is a reproduction of the bug: https://jsfiddle.net/qty0hLho/24/

holtkamp commented 6 years ago

mmm, indeed, this was fixed in Moment.js 2.12 which was released on 07-03-2016. Not sure we should "fix" this in this library? Or you in a situation where you can easily upgrade the used Moment.js version?

yphoenix commented 6 years ago

I'm going to upgrade our codebase to the latest version of the momentJS and have my QA team perform a full regression of everywhere it is used. Especially as there were some other issues that I found in the dateRangePicker that I haven't had time to investigate fully when monthSelect is set to true. I'm hoping the upgrade will fix those, other wise I'll investigate further.

Applying this fix to the library seems harmless enough as it bullet proofs it for people who are forced to use a legacy version of momentJS. That or update the library requirements to require 2.12+ instead of the current requirement of 2.8.1+. As we were already running 2.9 I knew we met the requirements.