Skip to content

fix(material/input): input harness not matching matNativeControl #18221

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

Merged
merged 3 commits into from
Jan 24, 2020

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