-
Notifications
You must be signed in to change notification settings - Fork 6.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(MdInput): Input should not be treated as empty if it is a date field. #846
Conversation
@@ -35,7 +35,7 @@ | |||
|
|||
<label class="md-input-placeholder" | |||
[attr.for]="inputId" | |||
[class.md-empty]="empty" | |||
[class.md-empty]="empty && type !== 'date'" |
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.
This should be better handled inside of the "empty" getter.
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.
Alright, I decided to do that in the template since I saw you guys did that with some other things. But it's fixed now in the latest commit.
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.
Any references? I guess in that cases there weren't any getters. If not the getter should probably not include that check.
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.
Well, these lines: https://github.com/drager/material2/blob/d8603948c36de9718e39284e58d12665a53ca569/src/components/input/input.html#L41-L42 as well as line 9.
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.
Ah thx, yeah that's a difference, since this is just an inline check. And the given variable / getter is only returning the value and not the boolean.
empty
is actually a property, which returns a boolean, which means that the check with the date
is part of the is empty
check.
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.
Alright, fair enough. The code is updated and correct then.
This is probably satisfactory to at least not be bad for @hansl any thoughts? |
I can probably get some unit tests done tomorrow. |
Please add a unit test and I'll approve. Thanks! |
09de981
to
22b0576
Compare
CI is failing because IE11 actually throws an error if you try to set |
Well look at that... http://caniuse.com/#search=date Yeah it's fine. Disable the tests on IE11. Unfortunate but IE11. |
@hansl: Alright, is this something that I should do? If so, how do I disable tests for a specific browser? |
@drager We don't currently have a way to do it. For now I think you can use the |
This should probably apply to type=time too? I noticed it does that same thing. |
f48cd96
to
a0b2e26
Compare
@jelbourn, @hansl: I guess this is ready to get merged if you accept the changes. Regarding @jmcgoldrick's issue, do you guys think this PR also should include that? I can fix that as well. |
Any updates on this PR? |
LGTM. Rebase and we'll merge. Thanks! |
928396e
to
ba12423
Compare
The failing tests are flaky, don't worry about them. thanks! |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Fixes #845