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

Commit

Permalink
fix(datepicker): handle correctly min/max when cleared
Browse files Browse the repository at this point in the history
  • Loading branch information
bekos authored and pkozlowski-opensource committed Jun 30, 2013
1 parent 1c99039 commit 566bdd1
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 3 deletions.
6 changes: 3 additions & 3 deletions src/datepicker/datepicker.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,13 +47,13 @@ angular.module('ui.bootstrap.datepicker', [])

if (attrs.min) {
scope.$parent.$watch($parse(attrs.min), function(value) {
minDate = new Date(value);
minDate = value ? new Date(value) : null;
refill();
});
}
if (attrs.max) {
scope.$parent.$watch($parse(attrs.max), function(value) {
maxDate = new Date(value);
maxDate = value ? new Date(value) : null;
refill();
});
}
Expand Down Expand Up @@ -161,7 +161,7 @@ angular.module('ui.bootstrap.datepicker', [])

scope.$watch('model', function ( dt, olddt ) {
if ( angular.isDate(dt) ) {
selected = angular.copy(dt);
selected = new Date(dt);
}

if ( ! angular.equals(dt, olddt) ) {
Expand Down
24 changes: 24 additions & 0 deletions src/datepicker/test/datepicker.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -507,6 +507,20 @@ describe('datepicker directive', function () {
}
}
});

it('enables everything before if it is cleared', function() {
$rootScope.mindate = null;
$rootScope.date = new Date("December 20, 1949");
$rootScope.$digest();

clickTitleButton();
for (var i = 0; i < 4; i ++) {
for (var j = 0; j < 3; j ++) {
expect(getOptionsEl(i, j).find('button').prop('disabled')).toBe( false );
}
}
});

});

describe('max attribute', function () {
Expand Down Expand Up @@ -580,6 +594,16 @@ describe('datepicker directive', function () {
}
}
});

it('enables everything after if it is cleared', function() {
$rootScope.maxdate = null;
$rootScope.$digest();
for (var i = 0; i < 5; i ++) {
for (var j = 0; j < 7; j ++) {
expect(getOptionsEl(i, j).find('button').prop('disabled')).toBe( false );
}
}
});
});

describe('date-disabled expression', function () {
Expand Down

18 comments on commit 566bdd1

@jojosati
Copy link

Choose a reason for hiding this comment

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

Does scope.model also need to handle before using it ?

Any idea if there is optional "storeFormat" for [ "valueOf", "UTC", "toDateString", "toISOString", "toJSON", "toLocaleDateString", ...] or any custom format like "yyyy-MM-dd" that Date( ) can parse correctly.

scope.model = new Date(selected);
if (storeFormat) {
    scope.model = angular.isFunction(scope.model[storeFormat])? scope.model[storeFormat]( ) : scope.model.format(storeFormat);
}

@bekos
Copy link
Contributor Author

@bekos bekos commented on 566bdd1 Jul 8, 2013

Choose a reason for hiding this comment

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

@jojosati Can you help me because I don't quite understand your question? Do you want to parse the selected date into a specific string format?

@jojosati
Copy link

Choose a reason for hiding this comment

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

@bekos excuse me, I 've just made you confuse.
It's hard for me to explain in english.

I have 2 points.

  1. The source from ngModel - scope.model,

I think now it accepts only native Date type.
If I store the date value in ISOString, it does not work.

Does it need to use Date(scope.model) to handle before using it (like you've done with min/max) ?

  1. The result from selected.

If you support any format of date that Date() can parse as binded model.
Why not you have an option to deliver any format of date as a result ?

@jojosati
Copy link

Choose a reason for hiding this comment

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

I've played with many changes to your datepicker.
jojosati@c9bb971

And example possibility to retrieve any form of date value, and put back a desired form of date value.
http://plnkr.co/edit/gZRHGmjIJw6hL8wWryMC?p=preview

@bekos
Copy link
Contributor Author

@bekos bekos commented on 566bdd1 Jul 9, 2013

Choose a reason for hiding this comment

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

@jojosati Good job and thx for sharing. Actually I have been playing around with it myself, but at this moment there is an open issue about plugging into the ngModelController (#612). Also, there was a similar issue very recently, where you can read @pkozlowski-opensource answer (#628).

Can you tell me your use case? For example if you want to assign your selected date into an input with a specific format you do something like this: http://plnkr.co/edit/5syqgua3DWrQ9qCUZJwN?p=preview

@pkozlowski-opensource
Copy link
Member

Choose a reason for hiding this comment

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

@bekos I don't know how you feel about changes proposed by @jojosati but my personal opinion is:

  • Regarding (1): this is not the configuration style we promote in this project. DOM attributes should be directive's public API. This is also not how AngularJS directives works - AFAIK there is no single directive in the AngularJS core that uses this style of configuring things.
  • Regarding (2) and (3): I strongly believe that data parsing / formatting should not be responsibility of the datepicker directive. We should plug it into NgModelController so people can support any formats and formatting styles. We've seen this already in Angular-UI and @petebacondarwin has more to say about it but just go over issues that was caused by in-directive parsing.

@jojosati
Copy link

Choose a reason for hiding this comment

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

@pkozlowski-opensource please excuse for queering code ahh.

ngModelController sounds good, but I don't know much now.

IMO most of date related tools or library in javascript, when dealing with date value as input.

It means any kind of date format that Date object can instantiate.

Many programers expect this and feel comfortbly passing a date value in their favor.

Even though datepicker retrieves date value from ngModelController, there is no need to strict to accept only Date object as input.

Another reason, I guess the min / max option accepted variety of date value via calling 'new Date(xxx)'.

Why not do the same thing with the input date value and let ngModelController do the job with informal date value.

oh... excuse me, my english is very poor.

@pkozlowski-opensource
Copy link
Member

Choose a reason for hiding this comment

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

@jojosati I'm not saying that we shouldn't make it easy to deal with dates expressed as strings. I'm just saying that we shouldn't do this inside the current date-picker directive. Rather we should provide an additional directive (used as an attribute) to have conversions for people that want to keep strings in they model.

If we hard-code those conversions in the date-picker directive you wouldn't have any means of customizing it and this would be bad. NgModelController with its parsers / formatters infrastructure was designed specifically to handle those type of conversions.

In short - yes for supporting dates as strings easily but no for hard-coding conversion logic inside the date-picker directive.

If there is anything left to discuss let's move the conversation to #612

@petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented on 566bdd1 Jul 10, 2013 via email

Choose a reason for hiding this comment

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

@bekos
Copy link
Contributor Author

@bekos bekos commented on 566bdd1 Jul 10, 2013

Choose a reason for hiding this comment

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

@petebacondarwin I am actually trying to rewrite this to use NgModelController and it's not that far, but I might want a bit help with the isolated scope. I have a similar problem like here.
I have managed to make this work with ng-change etc but only if I do: ng-model=$parent.date & ng-change="$parent.whatever()".

An option is to drop the isolated scope, but I don't consider this as the first one. I will prepare a plunker to see this and probably you or anyone else can provide a workaround.

@petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented on 566bdd1 Jul 10, 2013 via email

Choose a reason for hiding this comment

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

@pkozlowski-opensource
Copy link
Member

Choose a reason for hiding this comment

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

@bekos yes, you will have to do "scope magic" manually. I can help if needed, did it for the typeahead directive

@petebacondarwin
Copy link
Member

@petebacondarwin petebacondarwin commented on 566bdd1 Jul 10, 2013 via email

Choose a reason for hiding this comment

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

@bekos
Copy link
Contributor Author

@bekos bekos commented on 566bdd1 Jul 10, 2013

Choose a reason for hiding this comment

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

This is a WIP but just to show the ngModel and the scope problem. http://plnkr.co/edit/57deHWB9ML0IFz3CTzzD?p=preview

@pkozlowski-opensource I will look into the typeahead and ask for help if needed :-)

@joshkurz
Copy link
Contributor

@joshkurz joshkurz commented on 566bdd1 Jul 11, 2013 via email

Choose a reason for hiding this comment

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

@bekos
Copy link
Contributor Author

@bekos bekos commented on 566bdd1 Jul 11, 2013

Choose a reason for hiding this comment

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

I have updated the plunker (http://plnkr.co/edit/57deHWB9ML0IFz3CTzzD?p=preview)

Still WIP but two words about the current status (@petebacondarwin's approach). Let the datepicker directive as is, with the isolated scope etc. Only difference is an added select handler.

Created a bsDate directive that creates inside it the companion datepicker element and it's isolated scope, and a format directive like the one from angular-ui to parse/format the strings. If someone has time and the mood please take a look :-)

@pkozlowski-opensource
Copy link
Member

Choose a reason for hiding this comment

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

@bekos Maybe push it to GitHub & open a PR - will be easier to discuss on the particular code fragments

@bekos
Copy link
Contributor Author

@bekos bekos commented on 566bdd1 Jul 11, 2013

Choose a reason for hiding this comment

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

@pkozlowski-opensource You are right.

Please sign in to comment.