Skip to content
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

Merged
merged 20 commits into from
Sep 6, 2016

Conversation

drager
Copy link
Contributor

@drager drager commented Jul 12, 2016

Fixes #845

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jul 12, 2016
@@ -35,7 +35,7 @@

<label class="md-input-placeholder"
[attr.for]="inputId"
[class.md-empty]="empty"
[class.md-empty]="empty && type !== 'date'"
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@devversion devversion Jul 12, 2016

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.

Copy link
Contributor Author

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.

@jelbourn
Copy link
Member

This is probably satisfactory to at least not be bad for type="date". It does need a unit test, though.

@hansl any thoughts?

@drager
Copy link
Contributor Author

drager commented Jul 13, 2016

I can probably get some unit tests done tomorrow.

@hansl
Copy link
Contributor

hansl commented Jul 13, 2016

Please add a unit test and I'll approve. Thanks!

@hansl hansl changed the title fix(MdInput): Input should not be treated as empty if it is a date field. Fixes #845 fix(MdInput): Input should not be treated as empty if it is a date field. Jul 13, 2016
@jelbourn
Copy link
Member

jelbourn commented Jul 15, 2016

CI is failing because IE11 actually throws an error if you try to set type="date" on an <input>. In this case, I think it would be valid to skip this test for IE11. @hansl, do you have any opinions on this?

@hansl
Copy link
Contributor

hansl commented Jul 16, 2016

Well look at that... http://caniuse.com/#search=date

Yeah it's fine. Disable the tests on IE11. Unfortunate but IE11.

@drager
Copy link
Contributor Author

drager commented Jul 16, 2016

@hansl: Alright, is this something that I should do? If so, how do I disable tests for a specific browser?

@jelbourn
Copy link
Member

@drager We don't currently have a way to do it. For now I think you can use the !(window.ActiveXObject) && "ActiveXObject" in window trick in the test (though still in an isInternetExplorer11 function) and we'll refactor it later.

@jmcgoldrick
Copy link

This should probably apply to type=time too? I noticed it does that same thing.

@drager
Copy link
Contributor Author

drager commented Jul 28, 2016

@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.

@ross-nordstrom
Copy link

Any updates on this PR?

@hansl
Copy link
Contributor

hansl commented Sep 6, 2016

LGTM. Rebase and we'll merge. Thanks!

@hansl hansl added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs cleanup / tests labels Sep 6, 2016
@hansl
Copy link
Contributor

hansl commented Sep 6, 2016

The failing tests are flaky, don't worry about them. thanks!

@hansl hansl merged commit fe2b493 into angular:master Sep 6, 2016
@drager drager deleted the fix-md-input-date branch September 7, 2016 06:29
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug(md-input): MdInput with type date cannot handle placeholders
7 participants