-
Notifications
You must be signed in to change notification settings - Fork 27.5k
feat(input): support types date, time, datetime-local, month, week #5864
Conversation
Side note, it wasn't good that I was slamming Travis every time I had to update a deeper branch, because I'd also have to rebase the other branches "above" it and push them to Github. It was a mess. This should be cleaner. |
+1 |
Added a second commit to reduce the code size quite a bit: benlesh@952e08a I know that generally the Angular team would like single commits for a pull request, but given the size and scope of this PR, I'm leaving them both intact so they can be reviewed independently. I'll fix them up when it's necessary. |
It's actually okay to have multiple commits in your pull for larger changes, as long as they are sound units, and it adds to understanding them. They can be squashed before merging them. It was Igor or Brian who wrote this a few weeks ago in another pull. |
* | ||
* @description | ||
* HTML5 or text input with date validation and transformation. In browsers that do not yet support | ||
* the HTML5 date input, a text element will be used. The text must be entered in a valid ISO-8601 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add "In that case, the text must be entered ..." so it's clear that this is only the fallback case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for input widgets below
* @description | ||
* HTML5 or text input with date validation and transformation. In browsers that do not yet support | ||
* the HTML5 date input, a text element will be used. The text must be entered in a valid ISO-8601 | ||
* date format (yyyy-MM-dd), for example: `2009-01-06`. Will also accept a valid ISO date or Date object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you change this (doc and implementation) so that the model always needs to be a JS Date object?
If we allow a string initially, then the data type of the variable in the model changes when the user first enters a value. By this, js code that uses that model property always needs to check which case it is, and this will most probably lead to bugs...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about this, but I left it as it is now because it's extremely common that I'm actually putting a date string I've recieved from an AJAX call into the model. It's very nice to have for the workflow:
- Get some JSON data via Ajax containing dates
- The dates in the JSON are going to be strings initially.
- They're generally displayed directly into forms.
- When they're sent back to the server, they're automatically sent back as strings.
Another precedent for this functionality is the date
filter, which allows for either Date
object or string
dates to be input, and it will still format the date properly.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand your point.
But this is not the right place to convert Strings from the backend to
Dates. A http interceptor or something similar would be more appropriate.
On Tue, Feb 4, 2014 at 2:08 PM, Ben Lesh [email protected] wrote:
In src/ng/directive/input.js:
@@ -89,7 +94,345 @@ var inputType = {
*/
'text': textInputType,
- /**
\* @ngdoc inputType
\* @name ng.directive:input.date
*
\* @description
\* HTML5 or text input with date validation and transformation. In browsers that do not yet support
\* the HTML5 date input, a text element will be used. The text must be entered in a valid ISO-8601
\* date format (yyyy-MM-dd), for example: `2009-01-06`. Will also accept a valid ISO date or Date object
I thought about this, but I left it as it is now because it's extremely
common that I'm actually putting a date string I've recieved from an AJAX
call into the model. It's very nice to have for the workflow:
- Get some JSON data via Ajax containing dates
—
Reply to this email directly or view it on GitHubhttps://github.com//pull/5864/files#r9441282
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered the same thing. However, this seems like the easiest place to do it, because this is a point where you have a string that was just identified, probably for the first time, as being a Date. At any point prior to this in it's life cycle, there just really isn't an efficient way of identifying it, without explicitly mapping conversions, or inefficiently traversing the object looking for dates to convert.
In order for Angular to be buttery-smooth with Dates and these HTML5 controls, some sort of seamless conversion is nearly a must. Would you like me to put in a PR or two for some options regarding converting strings to dates?
I actually have a service I made to convert a string to a date, taking into account $locale
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at it again, the documentation is outdated and inaccurate. It looks like I removed the functionality that was scrubbing strings into dates when coming in from the model some time ago...
Still.. my offer to produce a few spikes for a date converter, or some date tools stands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... and... looking again, it seems to allow it even though I'm not explicitly changing it... I'll investigate that further and implement tests around making sure that only Date objects are accepted from the model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes,
a PR would be nice. However, please don't leave it in the directive.
On Fri, Feb 7, 2014 at 8:56 AM, Ben Lesh [email protected] wrote:
In src/ng/directive/input.js:
@@ -89,7 +94,345 @@ var inputType = {
*/
'text': textInputType,
- /**
\* @ngdoc inputType
\* @name ng.directive:input.date
*
\* @description
\* HTML5 or text input with date validation and transformation. In browsers that do not yet support
\* the HTML5 date input, a text element will be used. The text must be entered in a valid ISO-8601
\* date format (yyyy-MM-dd), for example: `2009-01-06`. Will also accept a valid ISO date or Date object
I considered the same thing. However, this seems like the easiest place
to do it though, because this is a point, where you have a string that was
just identified, for the first time, as being a Date. At any point prior to
this in it's life cycle, there just really isn't an efficient way of
identifying it, without explicitly mapping conversions, or inefficiently
traversing the object looking for dates to convert.In order for Angular to be buttery-smooth with Dates and these HTML5
controls, some sort of seamless conversion is nearly a must. Would you like
me to put in a PR or two for some options regarding converting strings to
dates?I actually have a service I made to convert a string to a date, taking
into account $locale.—
Reply to this email directly or view it on GitHubhttps://github.com//pull/5864/files#r9546086
.
This is pretty good! I really look forward to having these new input types in master soon! |
'HH:mm'), | ||
|
||
/** | ||
* @ngdoc inputType |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ngdoc input
So I guess that browsers just default to a text type if they don't recognise the type attribute? |
Correct. In fact, if you were to write a polyfill, that's precisely what you check var input = document.createElement('input');
input.type = 'banana';
if(input.type === 'text') {
console.log('browser does not have HTML7 banana input!');
} |
There is quite a bit of change to be made in the ngdoc comments due to the new Dgeni generated documentation. I did a rebase locally and there were a number of conflicts to fix up due to this. Sorry. |
Sorry for what? Haha. These things happen. Things get updated, other things need refactored. That's what that DEL is for. Shouldn't be terrible for me to rebase and make the changes. I'll try to make time over the weekend. |
Build is passing locally for me and once I modified one of the examples, it works fine in the docs. I would say that after a rebase, this is ready to go. |
Here's my rebased branch if you want to use it as a starting point: petebacondarwin/angular.js@angular:master...petebacondarwin:date-inputs |
Shall I just add it as an upstream of my own branch? |
@petebacondarwin and @tbosch ... I've rebased the branch, and I've updated all of the e2e tests in the documentation to use protractor. It's a good thing I did too, because I think there are going to be some confused developers when they go to use web driver to "sendKeys" to an HTML5 input like one of these (or even one like color), because it doesn't really work and now the example tests show the workaround. |
Thanks @Blesh! Keep up the good work!! |
👍 \o/ *confetti* |
Cool! Whenever I get done collaberating with @juliemr around angular/protractor#562, I'll create a new issue and PR with updated e2e tests in the docs that leverage the new ptor extension. |
On older browser that don't support the new HTML5 inputs and display a text input instead, the user is required to enter the data in the corresponding ISO format. The value in `ng-model` will always be a date. E2e tests contain a workaround to a bug in webdriver, see angular/protractor#562. Also adds weeks as format to the `dateFilter`. Related to angular#757. Closes angular#5864.
partially closes #757
Merged from PR #5256