Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

fix(datepicker): min-date/max-date: parse fix for literals #4841

Closed
wants to merge 1 commit into from

Conversation

davious
Copy link
Contributor

@davious davious commented Nov 7, 2015

fixes #3437

original plunker, with pr code: http://plnkr.co/edit/jsfOVBZj51yajpZYkC9N?p=preview
maintainer plunker, with pr code: http://plnkr.co/edit/buQXUxndajMYFOZorzq9?p=preview

Review on Reviewable

@davious davious force-pushed the 3437MinDateLiteral branch from c999d16 to e0673b0 Compare November 7, 2015 23:51
@davious davious changed the title fix(datepicker): min-date: strip time on literals fix(datepicker): min-date: timezone fix for literals Nov 7, 2015
@davious davious force-pushed the 3437MinDateLiteral branch from e0673b0 to be83344 Compare November 8, 2015 00:53
@wesleycho
Copy link
Contributor

Fix formatting - an easy way to see what is off is to rebase off of current master, run npm install, and run grunt eslint.

var date = parseDate(value);
if(!date) {
var tryDate = new Date(value);
if(angular.isDate(tryDate) && !isNaN(tryDate)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens when isNaN(tryDate) is true?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

undefined gets set.... this is to cover for an invalid attribute; example: a scope variable that doesn't exist.

@davious davious force-pushed the 3437MinDateLiteral branch from be83344 to f2c1906 Compare November 8, 2015 06:12
@davious
Copy link
Contributor Author

davious commented Nov 8, 2015

Not sure what formatting needs fixing. I rebased and ran grunt eslint: Done, without errors

@wesleycho
Copy link
Contributor

As a rule of thumb, the code should look like the surrounding code - the whitespace differences are pretty stark in this case, or lack of whitespace.

@wesleycho
Copy link
Contributor

Thinking about this problem for a bit, I am not sure we want to support this date format in the long run - it would be much better if it required a date object from the user's side as a breaking change, which would prevent the timezone issue, and allow the user the flexibility to specify the timezone of the min date.

@davious davious force-pushed the 3437MinDateLiteral branch from 9b425ab to 5cef4c8 Compare November 9, 2015 01:43
if (!date) {
date = new Date(value);
if (angular.isDate(date) && !isNaN(date)) {
var dateArray = date.toISOString().split('T')[0].split('-');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One can get the information from the date via this mechanism:

var year = date.getFullYear(),
  month = date.getMonth(),
  day = date.getDate();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the reason I'm using the toISOString() (I plan to switch to equivalent toJSON() next commit) is to get the UTC version of the date, otherwise there is the same timezone issue we are seeing in #3437... the date for EST looks something like 9PM the day before, and the date becomes off by 1.

if (!date) {
date = new Date(value);
if (angular.isDate(date) && !isNaN(date)) {
var dateArray = date.toJSON().split('T')[0].split('-');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As stated prior, one can get the information from the date via this mechanism:

var year = date.getFullYear(),
  month = date.getMonth(),
  day = date.getDate();

This is a better way of getting the date information

@davious
Copy link
Contributor Author

davious commented Nov 27, 2015

Changed the code to simply parse dates:

  • We now expect literal min-date and max-date to match the displayFormat; otherwise, it will not work at all
  • If they want to use an alternate format, they can indicate it in alt-input-formats

This fixes the underlying problem in issue #3437, which is that javascript parses yyyy-MM-dd as GMT; see #3437 (comment)

@davious davious force-pushed the 3437MinDateLiteral branch 3 times, most recently from 8aaa918 to 96f36d0 Compare November 27, 2015 15:46
@davious
Copy link
Contributor Author

davious commented Nov 27, 2015

Just realized that the concepts of display format and alt-input-formats does not exist for the datepicker itself. So, this would still be an issue for the datepicker proper; and, the different behavior of min-date/max-date would be confusing.

@davious davious changed the title fix(datepicker): min-date: timezone fix for literals fix(datepicker): min-date/max-date: parse fix for literals Nov 28, 2015
@davious
Copy link
Contributor Author

davious commented Nov 28, 2015

OK. Switched to using dateFilter to feed new Date for these literal cases.

@davious
Copy link
Contributor Author

davious commented Dec 2, 2015

@wesleycho would you re-review?

@wesleycho
Copy link
Contributor

Only thing I hesitate on is the assumption of the medium format in the dateFilter usage - I guess I'm ok with that, but it needs to be mentioned in the documentation.

@davious
Copy link
Contributor Author

davious commented Dec 14, 2015

The medium format is an internal format picked "at random"... Any format that indicates all the possible date information would do the trick. It doesn't impact the parse work.

@wesleycho
Copy link
Contributor

Guess I'm fine with it - LGTM then

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

min-date attribute does not disable the day prior to the given min-date
2 participants