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

Fix changes to month dropdown #421

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

yphoenix
Copy link

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.

When using the month dropdown option changes don’t take effect because
moment(opt[type])[name](value) 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
Copy link
Collaborator

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

@yphoenix
Copy link
Author

yphoenix commented Mar 26, 2018

Only happens with older versions of the MomentJS Library. Turns out it is fixed in 2.12 (moment/moment#2897)

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

@holtkamp
Copy link
Collaborator

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

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.

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.

2 participants