Datepicker: Ticket 7362, range checks allow you to go past yearRange, which causes weird glitches #271

Closed
wants to merge 3 commits into from

4 participants

@fracmak

Updated the range tests so you can't navigate past the yearRange which causes weird behavior where the UI where you're actually in a year that's not displayable, so clicking on a year gives you a date you aren't expecting. Unit tests included

@scottgonzalez
jQuery Foundation member

Shouldn't _getMinMaxDate() take the year range into account?

@fracmak

Well, I was kind of torn by this, because the documentation for yearRange is as follows: "Note that this option only affects what appears in the drop-down, to restrict which dates may be selected use the minDate and/or maxDate options." But the fact you can navigate left and right past the range of drop downs and end up thinking you're selecting one date and you really are selecting a different date is problematic. The _isInRange() function was a nice middle ground that didn't affect too much code. If you want to include yearRange into the minMaxDate, I'll gladly update the code and fix the documentation.

@scottgonzalez
jQuery Foundation member
@fracmak

Oops, sorry about that, slipped into the checkin by accident. Pull request updated

@gnarf
jQuery Foundation member

Might it be a smarter approach to make sure that minDate / maxDate get set to the start/end of the year range in the drop down if they aren't already set? And also make sure that the yearRange doesn't go outside of minDate/maxDate?

@scottgonzalez
jQuery Foundation member

Unfortunately any change like this would be a breaking change and therefore can't be done (it wouldn't make sense to make a change that can't go into 1.8.x since datepicker is being rewritten from scratch).

@scottgonzalez
jQuery Foundation member

The test fails when run in the full suite. It passes when run on its own (or if it's the first test to run, which happens after a failure).

@fracmak

is this something you want me to look into? or hold off on since it's not going into the 1.8 branch and the datepicker is being rewritten from scratch?

@scottgonzalez
jQuery Foundation member

You must've misunderstood my comment. When I said it would be a breaking change, I was replying to @gnarf37's comment about the more logical interaction of options. If you fix the failing unit test, this will land in the 1-8-stable branch.

@fracmak

Ok, the unit test is fixed. This checkin in a little bigger than before because I realized while investigating the problem that I hadn't done the yearRange calculation correctly, so I refactored the other year range code into it's own function that I could call. Ended up simplifying a lot of code. But this has had a minor side effect where if you set yearRange = "", the year range suddenly becomes the current year and you can't navigate anymore. This is how the existing year range calculation works, but this change did break the options:miscellaneous unit test because it was relying on html being generated by a successful call to _isInRange(). I've updated the test so it doesn't fail anymore. Let me know if this isn't intended behavior or if I missed anything.

@mikesherov
jQuery Foundation member

Jay, can you rebase this? I know it's very old at this point, but I'm trying to clean up the pull request queue. I got rid of datepicker_tickets.js, so now the test should live in the corresponding file the bug effects (either, methods or options or events). Thanks man!

@mikesherov
jQuery Foundation member

@fracmak, I just realized I never pinged you when commenting on this. Ping!

fracmak added some commits May 13, 2011
@fracmak fracmak Updated the range tests so you can't navigate past the yearRange whic…
…h causes weird behavior where the UI and what you select become out of sync. Unit tests included
4eeab67
@fracmak fracmak made unit test description better d3f4ad2
@fracmak

Imagine my surprise when rebasing a branch that was over a year old and it seriously merged cleanly except for that one deleted file. I moved the unit test to the minMax unit test. All datepicker tests are passing.

@mikesherov
jQuery Foundation member

Thanks @fracmak . Landed in eca5abd

A small note: I had to make a minor edit because JSHint was failing, but this is understandable but when this PR was authoring, we weren't yet JSHinting this file!

@mikesherov mikesherov closed this Nov 14, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment