Skip to content

Conversation

@devversion
Copy link
Member

The input harness currently only matches [matInput], but does
not match [matNativeControl] usages. Since both names, resolve
to the same directive, we should ensure that the harness works for both
possible usages.

@devversion devversion added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: patch This PR is targeted for the next patch release labels Jan 20, 2020
@devversion devversion added this to the 9.0.0 milestone Jan 20, 2020
@devversion devversion requested a review from mmalerba as a code owner January 20, 2020 09:48
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jan 20, 2020
Copy link
Contributor

@mmalerba mmalerba left a comment

Choose a reason for hiding this comment

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

Do we want this to create harness instances for <select matNativeControl>? That might be unexpected from a user's point of view. If we do decide to intentionally include it we should make sure its included in the tests as well.

@devversion
Copy link
Member Author

@mmalerba My thinking is that it wouldn't hurt to make the input harness work for these elements using matNativeControl. The harness exposes useful functionality for features of the directive.

I could also see having something like MatNativeControlHarness which simply has a different selector but extends MatInputHarness. Regarding tests, the PR includes a test for matNativeControl, or do you think of something else?

@mmalerba
Copy link
Contributor

I mean a test with a <select> element. I think I'm ok with including it, but I'm curious if @jelbourn has an opinion

@devversion
Copy link
Member Author

@mmalerba I looked more into it, and it looks like the scenario with select you brought up is slightly different than usual inputs:

  • setValue will not work like for text inputs. We could add additional logic for native select elements.
  • getPlaceholder currently relies on the host element .placeholder property. We could switch that to use the attribute.. though does it make sense to have such a method for native selects?

Also the more I think about it, it sounds wrong to have native select elements under MatInputHarness. The select is not necessarily an input (in terms of HTML elements).

@mmalerba
Copy link
Contributor

Ok, that's what I was afraid of. In that case lets make the selector [matInput],input[matNativeControl]. We can make a separate harness for native select, or try to roll it into MatSelectHarness

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Jan 23, 2020
…ntrol`

The input harness currently only matches `[matInput]`, but does
not match `[matNativeControl]` usages. Since both names, resolve
to the same directive, we should ensure that the harness works for both
possible usages.
@devversion devversion force-pushed the fix/input-harness-selector branch from 4d0a517 to 5c3296f Compare January 23, 2020 09:04
@devversion
Copy link
Member Author

@mmalerba Addressed feedback. Can you please have another look?

@mmalerba
Copy link
Contributor

Oh also textarea[matNativeControl], almost forgot about that one

@mmalerba mmalerba removed the action: merge The PR is ready for merge by the caretaker label Jan 23, 2020
Copy link
Contributor

@mmalerba mmalerba left a comment

Choose a reason for hiding this comment

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

LGTM other than the missing selector - add merge-ready when done

@devversion
Copy link
Member Author

Fixed. Also added it to the tests.

@devversion devversion added the action: merge The PR is ready for merge by the caretaker label Jan 23, 2020
@jelbourn jelbourn merged commit fe0e963 into angular:master Jan 24, 2020
jelbourn pushed a commit that referenced this pull request Jan 24, 2020
…18221)

* fix(material/input): input harness selector not matching `matNativeControl`

The input harness currently only matches `[matInput]`, but does
not match `[matNativeControl]` usages. Since both names, resolve
to the same directive, we should ensure that the harness works for both
possible usages.

* fixup! fix(material/input): input harness selector not matching `matNativeControl`

Address feedback

* fixup! fixup! fix(material/input): input harness selector not matching `matNativeControl`

Address feedback

(cherry picked from commit fe0e963)
yifange pushed a commit to yifange/components that referenced this pull request Jan 30, 2020
…ngular#18221)

* fix(material/input): input harness selector not matching `matNativeControl`

The input harness currently only matches `[matInput]`, but does
not match `[matNativeControl]` usages. Since both names, resolve
to the same directive, we should ensure that the harness works for both
possible usages.

* fixup! fix(material/input): input harness selector not matching `matNativeControl`

Address feedback

* fixup! fixup! fix(material/input): input harness selector not matching `matNativeControl`

Address feedback
@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 Feb 24, 2020
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 P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants