feat(datepicker): convert string to Date with custom date formats #1430

dnasir · 2013-12-17T22:42:24Z

Added filter that converts string to Date object.
Added relevant tests.

2. Added relevant tests.

bekos · 2013-12-17T23:32:54Z

@dnasir Have not reviewed code yet but looking at the tests this seems pretty awesome to me! 👍

bekos · 2013-12-18T13:26:38Z

src/datepicker/datepicker.js

-.directive('datepickerPopup', ['$compile', '$parse', '$document', '$position', 'dateFilter', 'datepickerPopupConfig', 'datepickerConfig',
-function ($compile, $parse, $document, $position, dateFilter, datepickerPopupConfig, datepickerConfig) {
+.directive('datepickerPopup', ['$compile', '$parse', '$document', '$position', '$filter', 'dateFilter', 'datepickerPopupConfig', 'datepickerConfig',
+function ($compile, $parse, $document, $position, $filter, dateFilter, datepickerPopupConfig, datepickerConfig) {


@dnasir Instead of $filter you can inject just stringToDateFilter

bekos · 2013-12-18T13:51:45Z

@dnasir I have left some comments on the code. I would also like you to test the filter itself. Probably we should extract this into a separate module and add as dependency into datepicker.

dnasir · 2013-12-18T20:04:46Z

@bekos Thanks for the code review. I'll make the necessary changes and resubmit the pull request.

About the filter itself, I do agree that it makes more sense to have the filter located somewhere else where other modules could also have access to it, as it's pretty much a general purpose string to date filter. Where do common or shared stuff go?

I have the filter code as an Angular filter in a separate repo located here: https://github.com/dnasir/angular-stringToDateFilter

bekos · 2013-12-18T20:14:21Z

@dnasir You can create a new folder. For example take a look at the $position service in the position folder.

dnasir · 2013-12-18T20:48:00Z

@bekos Awesome.Thanks for the tip.

2. Added relevant tests.

2. Added stringToDateFilter to datepicker as a dependency.

…atepicker_patch Conflicts: src/datepicker/datepicker.js

dnasir · 2013-12-20T06:57:00Z

@bekos As suggested, I've refactored the string to date filter as a separate module, and added the necessary tests. I've also implemented the module into the datepicker code.

pkozlowski-opensource · 2013-12-20T08:12:44Z

@dnasir @bekos I've looked at this PR only briefly, but I wonder if we really want to have this logic in this repo. Shouldn't we simply add a dependency (an optional one) on moment.js or a similar library?

Besides I'm really not clear why we should be using a filter to convert a string to Date - this is normally a role of a parser in inputs using ngModel. Taking moment.js and creating a simple directive we can have support for many date formats: https://github.com/IgorMinar/directives-workshop/tree/master/src/05_ngmodelctrl_parse_format/exercise

In short: I don't think is the right approach here and I would rather use a dependency on a well known library instead of coping & pasting it here.

bekos · 2013-12-20T16:27:49Z

@pkozlowski-opensource It's true that I was probably a lit biased about this PR :-)
I liked the idea of having a full working datepicker in our library, without external dependencies. We use AngularJS for formatting and validating dates but there is no in-house solution for parsing.

I understand, as you say, that this is a difficult task atm and we should rely on proven libraries for this, but this does not sound much as this project's philosophy :-)

Technically, my concern is that the formatting tokens used in momentjs or any other library and those in angularjs' dateFilter don't match exactly. Based on this, someone has to provide two different date format strings.
And if this is not much trouble, what if something is supported by angularjs and not by momentjs. Let's say, in an extreme case, that this happens with a locale.

@dnasir I did a bit of testing myself and seems that there are some bugs here. I can see the difficulty of this task, but I would suggest to keep working and improving your stuff. Probably Angular itself will be interested in a dateParser at some point in the future, in conjunction with the existing dateFilter.

My opinion is that if we have a proven code that works for parsing date's from the the angular's format, I would not have a problem including here. Otherwise we can make a sample external directive with dependency to momentjs.

dnasir · 2013-12-22T00:03:27Z

@bekos I think @pkozlowski-opensource is right. This should actually be a parser rather than a filter. I'll see what I can do, now that I'm on vacation and have more time to spend on open source projects ;-)

Also, what sort of bugs did you encounter? Please let me know so I could look into it.

@pkozlowski-opensource I did look into momentjs before I created my own code, but as mentioned by @bekos, the different format strings used by both libraries makes it a bit tedious to work with. I thought about creating some sort of format converter, but that seemed a bit silly.

julienmeyer · 2014-01-15T10:05:53Z

Thanks for your work @dnasir !
I'm agree with you about using different format strings so I will test your directive.

SpoBo · 2014-01-17T13:16:24Z

This is definitely a problem that needs fixing. Out of the box the datepicker directive does incorrect things when the locale in angular differs from the one in your browser. We're in need of this fix.

If I may make a suggestion though. I would not include this logic by default in bootstrap-ui because this is functionality that might be duplicated by another library that's already included. For example momentjs, datejs, sugarjs, etc.

The best solution would be that there is an intermediary (facade) service that has a parse(string, format) but more importantly a setAdapter(dateParserAdapter) function that accepts an adapter. An adapter is something written to bridge the difference between angularjs and an existing date library. It'll convert the format if needed and finally use its own internal parse function. Or the facade could convert the proprietary angularjs format to the shared javascript format.

The facade will then simply use the adapter that has been set. If no adapter is set the facade will simply do new Date(value). It'd be nice though if the parse code that's in this branch isn't included by default in order to keep the size of projects down that already include a dateparser for which an adapter exists.

It's a bit more cumbersome to set up but the most flexible solution. In any case angularjs feels a bit broken of there is no reverse for the dateFormatter.

chrisirhc · 2014-01-22T11:31:37Z

Regarding this functionality, people are likely to have different opinions on how the date strings are to be parsed. Can we instead ask users to add parsers/formatters on their own to ngModel? Does that work?
If it helps, we can add an FAQ for that?

Foxandxss · 2014-01-22T14:09:34Z

I am totally with @chrisirhc here. The whole idea is to give a working datepicker and then you adapt it to your needs. If we need to put all the users needs into it, that is not good at all.

You put a bunch of formatting rules, that is good, but I bet that there is someone who needs an extra rule and if we continue putting new rules, it will become a mess.

ngModel parsers/formatters are easy to apply and use. You just need to add the one you need to your input and YGTG.

Otoh, I like the idea of having a FAQ or maybe related tutorials for directives.

pkozlowski-opensource · 2014-01-22T19:15:32Z

@chrisirhc I'm on the fence here... The trouble is that AngularJS provides formatting capability out of the box so I would say it is natural to expect that parsing would be supported as well. Plus plugging into parsers / formatters is a pain in the (#&(&#$ atm (requires a directive etc.). I kind of see added value of this project if we can support AngularJS formats for both formatting and parsing.

mvhecke · 2014-01-22T20:11:58Z

I'm with @chrisirhc, because this way people can choose what they prefer. In my own project I use Moment.js for dates so that might be a good example, but just maybe I shouldn't take sides in this so I rest my case.

randallmeeker · 2014-01-22T20:12:11Z

I think the FAQ+1 is a great Idea, because you usually dealing very different use cases. I personally integrate with moment.js and pre/post format all my dates into strings stripped of locale info. I would not expect angular-ui to solve for my use case. (but it would be great if they could!!!)

Since people here are big users of the date/time objects, widgets, function and libraries, we should also be looking at the core angular library to help solve for these issues. My thought is that angular should build something into their library that allows for the selection of locale and their own date function. Then we can replace the date() object with a better $.date() object. In these directives.

bekos · 2014-01-22T20:30:35Z

I really don't understand the arguments here. This piece of code tries to solve one specific problem: if you provide a string into the the same format that you specified to be exported, it would be able to parse it correctly.
I don't think there are many different use cases we would like to support.

All the struggle to plug to ngModelController was to be able someone to override the default parsers/formatters and use momentjs or any other custom code. I personally haven't seen a datepicker that cannot parse the date from the input out of the box, and I also feel and like our library to support this also.

I don't think that we are trying to solve an non-existent problem here. For me it's +1 to be in the core.

bekos · 2014-01-22T20:32:18Z

Edit: I don't mean this specific piece of code - which should be reviewed - but the functionality.

randallmeeker · 2014-01-22T20:40:19Z

@bekos ya, I think I was off topic here. My bad.

chrisirhc · 2014-01-22T21:28:39Z

src/stringtodate/stringtodate.js

+angular.module('ui.bootstrap.stringToDate', [])
+/**
+ * A filter that converts string to JavaScript Date object using any provided filters.
+ * Valid formats can be found in the AngularJS date filter documentation.


I'm not sure what "using any provided filters" refers to here.

This comment should make it clear that this basically does the opposite of the date filter.

chrisirhc · 2014-01-22T22:12:57Z

@bekos , sorry I didn't look into this in detail before. This stringToDate kind of does the opposite of the AngularJS date filter. Previously, I didn't know that we were using the formatting syntax of the date filter. It then makes sense to write/use a parser for converting the view values to dates through that well-defined format specification.

Looks good actually.

chrisirhc · 2014-01-22T22:15:11Z

src/datepicker/datepicker.js

@@ -351,7 +350,7 @@ function ($compile, $parse, $document, $position, dateFilter, datepickerPopupCon
          ngModel.$setValidity('date', true);
          return viewValue;
        } else if (angular.isString(viewValue)) {
-          var date = new Date(viewValue);
+            var date = stringToDateFilter(viewValue, dateFormat) || new Date(viewValue);


Looks like the indentation is wrong.

bekos · 2014-02-23T17:25:05Z

@angular-ui/bootstrap I would really like to merge this one, except there is a fundamental objection?

@dnasir Do you still care to contribute to this one?

pkozlowski-opensource · 2014-02-23T17:28:52Z

@bekos I think the real question is this: can we make it small and robust / bug free? Date-manipulation code tends to be hard to write and not that easy to test but if you are confident with the code and it doesn't add 1000 LOC - go for it!

dnasir · 2014-02-24T07:43:48Z

@bekos I'll contribute when I have the time, but have a look at my date parser before you go ahead.

https://github.com/dnasir/angular-dateParser

It's a work in progress, and I need more tests, but I don't really have that much free time. I do think it's the way to go though.

bekos · 2014-04-13T19:04:53Z

@dnasir Thx for your work and this PR, but as you said this is not the way to go. There is already another PR (#1874) that tries to accomplish the same thing, in a cleaner way IMO, and I hope to be merged soon.

Thx again for your effort, and looking forward to another PR form you :-)

dnasir added 2 commits December 17, 2013 21:44

1. Added filter that converts string to Date object.

4a48448

2. Added relevant tests.

Applied JSHint fixes.

c5b0144

bekos reviewed Dec 18, 2013
View reviewed changes

dnasir added 4 commits December 19, 2013 18:49

1. Added filter that converts string to Date object.

17362d7

2. Added relevant tests.

Applied JSHint fixes.

eb045ec

1. Added stringtodate module.

e2e7bdd

2. Added stringToDateFilter to datepicker as a dependency.

Merge branch 'datepicker_patch' of github.com:dnasir/bootstrap into d…

9ae0cab

…atepicker_patch Conflicts: src/datepicker/datepicker.js

bekos mentioned this pull request Jan 15, 2014

bug(datepicker): input manually -> invalid or day/month is switched #956

Closed

ghost assigned chrisirhc Jan 19, 2014

chrisirhc reviewed Jan 22, 2014
View reviewed changes

rickirunge mentioned this pull request Feb 23, 2014

Bugfix: Datepicker doesn't mark input field invalid if the date is invalid #1845

Closed

getuliojr mentioned this pull request Feb 24, 2014

FIX - DatePicker now accepts input value fields with the right formatDate #1855

Closed

bekos closed this Apr 13, 2014

feat(datepicker): convert string to Date with custom date formats #1430

feat(datepicker): convert string to Date with custom date formats #1430

Uh oh!

Conversation

dnasir commented Dec 17, 2013

Uh oh!

bekos commented Dec 17, 2013

Uh oh!

bekos Dec 18, 2013

Choose a reason for hiding this comment

Uh oh!

bekos commented Dec 18, 2013

Uh oh!

dnasir commented Dec 18, 2013

Uh oh!

bekos commented Dec 18, 2013

Uh oh!

dnasir commented Dec 18, 2013

Uh oh!

dnasir commented Dec 20, 2013

Uh oh!

pkozlowski-opensource commented Dec 20, 2013

Uh oh!

bekos commented Dec 20, 2013

Uh oh!

dnasir commented Dec 22, 2013

Uh oh!

julienmeyer commented Jan 15, 2014

Uh oh!

SpoBo commented Jan 17, 2014

Uh oh!

chrisirhc commented Jan 22, 2014

Uh oh!

Foxandxss commented Jan 22, 2014

Uh oh!

pkozlowski-opensource commented Jan 22, 2014

Uh oh!

mvhecke commented Jan 22, 2014

Uh oh!

randallmeeker commented Jan 22, 2014

Uh oh!

bekos commented Jan 22, 2014

Uh oh!

bekos commented Jan 22, 2014

Uh oh!

randallmeeker commented Jan 22, 2014

Uh oh!

chrisirhc Jan 22, 2014

Choose a reason for hiding this comment

Uh oh!

chrisirhc commented Jan 22, 2014

Uh oh!

chrisirhc Jan 22, 2014

Choose a reason for hiding this comment

Uh oh!

bekos commented Feb 23, 2014

Uh oh!

pkozlowski-opensource commented Feb 23, 2014

Uh oh!

dnasir commented Feb 24, 2014

Uh oh!

bekos commented Apr 13, 2014

Uh oh!

Uh oh!