Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix(ngModel): support milliseconds in time and datetime #8879

Closed
wants to merge 2 commits into from
Closed

fix(ngModel): support milliseconds in time and datetime #8879

wants to merge 2 commits into from

Conversation

shahata
Copy link
Contributor

@shahata shahata commented Sep 1, 2014

Closes #8874

@btford btford self-assigned this Sep 2, 2014
@btford btford added this to the 1.3.0-rc.1 milestone Sep 2, 2014
@btford
Copy link
Contributor

btford commented Sep 3, 2014

I like this change except that it means that milliseconds are always displayed. I'm wondering if it makes sense to hide them unless milliseconds are non-zero or the step attribute has a value less than one.

@shahata
Copy link
Contributor Author

shahata commented Sep 3, 2014

I remember checking it and seeing that chrome does not display the milliseconds (and also seconds) if it's zero. I'll take another look in the morning.

@shahata
Copy link
Contributor Author

shahata commented Sep 4, 2014

@btford chrome indeed does the right thing when milliseconds is zero and does not display it: http://plnkr.co/edit/hYZwvnFSMBUBBHQiBvBn?p=preview

In case you believe it is better we do not include the .000 fraction regardless, I added another commit to this PR that does that. (it's not pretty and I don't like it though)

@shahata
Copy link
Contributor Author

shahata commented Sep 4, 2014

oooh I forgot about the case with step attribute having a value which is less than one. I'll add it later today unless you let me know you no longer want it :)

@btford
Copy link
Contributor

btford commented Sep 4, 2014

@shahata thanks, looking into it now.

@shahata
Copy link
Contributor Author

shahata commented Sep 5, 2014

@btford I added the step case as well

@btford
Copy link
Contributor

btford commented Sep 10, 2014

Thanks, @shahata. I'll take a look at this once #8912 lands. I don't think it'll take too much rebasing, but I wanted to give you a heads up.

Verified

This commit was signed with the committer’s verified signature.
dlrobertson Dan Robertson
Closes #8874

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
1) milliseconds count is non-zero
2) step attribute is less than one
@shahata
Copy link
Contributor Author

shahata commented Sep 11, 2014

Rebased with #8912
Just wanted to mention again that I'm not big on the second commit of this PR.
IMO we should always include milliseconds and only the first commit needs to be merged.

@btford
Copy link
Contributor

btford commented Sep 22, 2014

After looking a bit more, I think your suggestion is good. I'm going to merge just 5811560 for now.

Thanks so much!

@btford
Copy link
Contributor

btford commented Sep 22, 2014

Landed as 4b83f6c. Thanks!

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

Successfully merging this pull request may close these issues.

Input[datetimeLocal] directive do not support changing second and millisecond.[version 1.3.0-beta.19]
5 participants