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

Datepicker doesn't parse manually edited input value using defined format #1107

Closed
Buthrakaur opened this issue Oct 1, 2013 · 7 comments
Closed

Comments

@Buthrakaur
Copy link

I'm using datepicker-popup attribute to set date format according to angular $locale service, which depends on browser language settings in my case. The read-only part of the problem works quite nice. The problem is there's no support for date parsing according to this format when user manually sets date in the input field instead of using datepicker popup. The problem is probably on this line:
https://github.com/angular-ui/bootstrap/blob/master/src/datepicker/datepicker.js#L348 - the value is parsed just using new Date(viewValue) instead of using the format defined in datepicker-popup attribute.

I'd happily fix this issue and make a PR, but I don't know about any easy possibility to parse date value using a format string in JS without using any external library. Any ideas?

@bekos
Copy link
Contributor

bekos commented Oct 1, 2013

@Buthrakaur I am glad you are interested to help with this issue that seems to confuse many people :-(

As you can see at the TODO some lines above the line you referenced, we need someone to implement a dateParser that does the reverse work of dateFilter. The best way to implement is probably to a separate directive that could be attached or not to the datepicker-popup to parse the date correctly.

You can start by supporting only a subset of the formats that dateFilter supports, for example only the symbols dd, mm and yyyy. You can find many date parse to borrow ideas and adjust to our needs. For example momentjs, jquery ui or even a simple one that supports the symbols I said earlier here.

@Buthrakaur
Copy link
Author

@bekos Thanks for hints - will take a look at it. It should be also interconnected with $locale service to resolve formats like mediumDate, which are most useful in multi-locale applications...

@pkozlowski-opensource
Copy link
Member

@Buthrakaur I hope that @bekos explanations were enough to help you solve this issue. We can't add parsing of custom data formats without introducing a dependency on a 3rd party library (moment.js or similar) and we want to keep dependencies of this project to the minimum. This is why custom data formats (that can't be passed by the Date constructor) need to be parsed by users of this directive.

@Buthrakaur
Copy link
Author

@pkozlowski-opensource Yes - the explanations were enough for me to understand cause of the issue. But to be honest I don't think it's a good idea to just close this issue. I completely understand your 3rd party dependency aversion, but the thing is the code isn't even extensible enough to plug in some external dependency by end users of angular-ui library. I think you should provide some proper extension point and mention it in docs at least to close this issue in correct way. Then angular-ui users would be really able to plug in their own choice of Date parsing algorithm based on moment.js or whatever else. Now this is not possible in any clean way without forking the code base.

@pkozlowski-opensource
Copy link
Member

@Buthrakaur oh, sorry, of course I didn't mean to "just close this issue". One of the issues referenced by @bekos (#956) has an example source code illustrating how to plug custom parsing, see this comment:
#956 (comment)

There is an extension point in the directive (well, to be precise, it exists in AngularJS in form of NgModelController $parsers pipeline: http://docs.angularjs.org/api/ng.directive:ngModel.NgModelController http://docs.angularjs.org/api/ng.directive:ngModel.NgModelController). So there is a proper extension point in place and you can plug into it without modifying a single line of the library's code.

I know that it would be great to document all this but we are just too stretched as this library getting more and more popular. So any help in form of a pull request would be highly appreciated!

In short: there is an extension point, there is no code change needed in the library and this is why this issue was closed. The only thing I can see as actionable it additional documentation and would appreciate help for this.

@Buthrakaur
Copy link
Author

@pkozlowski-opensource I see - thanks a lot for your explanation. I didn't know about this extension point. So excuse my previous unnecessarily too sharp comment, please :)

@mini-me
Copy link

mini-me commented Dec 19, 2013

Not sure that reinventing the well-written library is better than adding OPTIONAL dependency...

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

No branches or pull requests

4 participants