Skip to content

feat(material-experimental): add test harness for input #16674

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 23, 2019
Merged

feat(material-experimental): add test harness for input #16674

merged 1 commit into from
Aug 23, 2019

Conversation

devversion
Copy link
Member

@devversion devversion commented Aug 3, 2019 •
edited
Loading

  • See individual commits.

Blocked on #16706 and #16645

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 3, 2019
@devversion devversion marked this pull request as ready for review August 3, 2019 09:22
@devversion devversion requested a review from mmalerba as a code owner August 3, 2019 09:22
@devversion devversion requested a review from jelbourn as a code owner August 3, 2019 09:22
@devversion devversion added pr: merge safe target: patch This PR is targeted for the next patch release merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Aug 3, 2019
@devversion
Copy link
Member Author

It looks like there is one failing test on iOS because the keyup event by sendKeys does not set the event target. This should be fixed once #16645 lands.

Marking as blocked in the meanwhile. This PR can be still reviewed.

@devversion devversion added the blocked This issue is blocked by some external factor, such as a prerequisite PR label Aug 3, 2019
@googlebot

This comment has been minimized.

Sign in to view
@googlebot googlebot added cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Aug 7, 2019
@devversion devversion force-pushed the feat/input-harness branch 5 times, most recently from 60e36b0 to 82e0f14 Compare August 7, 2019 07:34
@devversion devversion removed the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Aug 7, 2019
jelbourn
jelbourn approved these changes Aug 15, 2019
View reviewed changes
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

@devversion
Copy link
Member Author

Addressed feedback. PR is currently still blocked on #16706

mmalerba
mmalerba reviewed Aug 15, 2019
View reviewed changes
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.

This looks good for now, but it'll be interesting to see how it plays with the form-field harness once we create that

src/material-experimental/mdc-input/harness/input-harness.ts
async setValue(newValue: string): Promise<void> {
const inputEl = await this.host();
await inputEl.clear();
// We don't want to send keys for the value if the value is an empty
Copy link
Contributor

Choose a reason for hiding this comment

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

should we update sendKeys to not do that?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure. I could see sendKeys being a noop in that case. Though it also feels like an anti-pattern in general to call sendKeys with an empty value.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, calling it with empty string is weird and people shouldn't do it, so I'm fine with changing it or just leaving it how it is

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd vote for leaving it as is, but maybe throwing an error in sendKeys instead? (follow-up)?

@mmalerba mmalerba added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Aug 15, 2019
@googlebot
Copy link

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@mmalerba mmalerba removed the blocked This issue is blocked by some external factor, such as a prerequisite PR label Aug 23, 2019
@mmalerba
Copy link
Contributor

This should be unblocked now that getProperty has landed

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@devversion
Copy link
Member Author

@mmalerba Rebased. can you have another look?

mmalerba
mmalerba approved these changes Aug 23, 2019
View reviewed changes
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 with a few nits

@devversion
feat(material-experimental): add test harness for input
5abda4a 
Adds a test harness for the `MatInput` implementation.

Resolves COMP-182
@devversion devversion added the action: merge The PR is ready for merge by the caretaker label Aug 23, 2019
@ngbot
Copy link

ngbot bot commented Aug 23, 2019

I see that you just added the pr: merge ready label, but the following checks are still failing:
    failure status "ci/circleci: tests_browserstack" is failing

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

jelbourn
jelbourn approved these changes Aug 23, 2019
View reviewed changes
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 merged commit ab9a847 into angular:master Aug 23, 2019
mmalerba pushed a commit to mmalerba/components that referenced this pull request Aug 27, 2019
@devversion @mmalerba
feat(material-experimental): add test harness for input (angular#16674)
9090474 
Adds a test harness for the `MatInput` implementation.

Resolves COMP-182
@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 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mmalerba mmalerba mmalerba approved these changes

@jelbourn jelbourn jelbourn approved these changes

Assignees

@jelbourn jelbourn

@mmalerba mmalerba

Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@devversion @googlebot @mmalerba @jelbourn