-
Notifications
You must be signed in to change notification settings - Fork 27.4k
Conversation
Thanks for the PR!
If you need to make changes to your pull request, you can update the commit with Thanks again for your help! |
I've signed the CLA in the past. Resigned today for good measure (I'm not sure if it's changed). PR commit messages should be in order (but I won't be hurt if they need tweaked). Looking forward to feedback. This is based off of work I had to do for another project, and it worked out well there. Even though there it was in a separate module and .js file. |
To be clear, this is not an attempt at a polyfill. It just adds basic validation handling and some binding niceties for Angular. |
Hi,
I agree this would be nice to have, but not within angular-core. |
Support older browsers... sure. how old? Which browsers? Support all locale bundles? Why does it need to support locale bundles? This is not a polyfill. It's an implementation to tie Angular validation to the HTML spec in a simple way. Nearly identical to the handling of the input of type "number". Implement a date parser for all those locales? Again, I'm not really sure I follow. The HTML spec states the ISO-8601 format is what's accepted (YYYY-MM-dd). The format is to the W3C HTML spec found here. Which states that output from input of type "date" should be ISO-8601 or "YYYY-MM-dd".
Was this a unilateral decision to close this or was there a discussion? It seems the spirit of the pull request has been missed. I'm willing to iterate on this pull request until there's something viable by Angular standards as a starting point. Additional reading: |
Hi, So yes, there was a discussion around this. Older browsers that don't support
If Angular supports If you want to add those missing peaces I would be happy to support you in this. |
Sounds great. I'll work on it.
|
|
||
var getDateValue = function(iso) { | ||
if(isDate(iso)) { | ||
return +iso; |
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 use getTime() instead to make the code readable.
I added a few comments to the PR. I tested the behavior of the date input and it appears that at least chrome formats the date using users current locale. this formatting doesn't seem to be configurable. From other browsers at least opera uses the ISO format instead of locale-specific formatting. While these inconsistencies are not great, the fact that we can't affect the formatting in chrome anyway makes me think that we could take this PR as is. the biggest issue I have with the current change is the |
I assume you're talking about the function name
Absolutely. It was added to the pull request as more of a proposal. Since the HTML spec says it wants ISO date strings, but parsing those out to dates is going to be a really common practice. Perhaps a date parsing service at a later time would be a better approach?
Sure. Didn't think about it.
@IgorMinar That's interesting, so you're saying the output of the input (ha) type "date" in Chrome isn't always ISO-8601? (yyyy-MM-dd)
For point of reference, I couldn't find (or I skimmed past, more likely) your formatting preferences for the project. I wanted to set my IDE (WebStorm) to have the same preference so auto-formatting doesn't blow up commits with superfluous line changes. I'm a compulsive auto-formatter so I've been fighting the urge. ;) Locale and formattingAs far as formatting goes, my plan for the next iteration on this is to leverage $locale and parse each of the format types, for example:
From those my plan is to either:
OR
... I'm on the fence with each approach. In both cases I'd check to see if the browser already has an implementation of the date input. Also, there is the consideration of how this would work for the "month" input type and the "datetime" input type. What are your thoughts? Other thingsI feel like the unit tests around this may need some hardening. I'm not sure most branches of this code have been covered. |
So, to be clear from that, do you want me to simply fix the issues you've specified in my current pull request branch and leave it? Do you want me to branch again and add the other functionality I mentioned in my previous post? OR... shall I add similar handling to this original PR for (or both I guess) |
I've made what I think are the requested changes. If this is good to merge with the current functionality, I'd like to go ahead and add the same basic functionality for |
@Blesh thanks a lot! And could you squash your commits into a single commit? Thanks! |
Lol... that commit to I will make the requested changes and squash this into a single commit. |
So, I've figured out the fix for the issue in #4595 and closed it. So the |
I think I've got it this time. Please review and let me know if this works. If so, I'll have input types |
+1 |
5 similar comments
+1 |
+1 |
+1 |
+1 |
+1 |
+1 |
👍 |
+1 |
14 similar comments
+1 |
+1 |
+1 |
+1 |
+1 |
+1 |
+1 |
+1 |
+1 |
+1 |
+1 |
+1 |
+1 |
+1 |
+1 |
3 similar comments
+1 |
+1 |
+1 |
partially closes angular#757
Can we close this PR now as you created #5864? |
Yes. This can be closed in favor of #5864 |
This is to at least partially address #757
The idea is to add support for existing HTML5 implementations of
<input type="date"/>
while supporting a text version for older browsers that handles the appropriate HTML5 validations.Additionally, an Angular-specific attribute was added (
ng-output-date
) to give the developer the option to output the value to the model as a Date object rather than the default string.All string are expected in the HTML spec compliant ISO-8601 date strings: 'YYYY-MM-DD'
Hopefully this lays the groundwork for other similar input types such at
datetime
,month
,datetime-local
, etc.