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.

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

fix(button): complete ripple when button becomes disabled #4372

Merged
merged 2 commits into from
May 22, 2017
Merged

fix(button): complete ripple when button becomes disabled #4372

merged 2 commits into from
May 22, 2017

Conversation

willshowell
Copy link
Contributor

Fixes #4364

@willshowell willshowell mentioned this pull request May 3, 2017
Closed
@willshowell willshowell force-pushed the feat/finish-ripple-disabled-button branch from 586ea8a to 038f355 Compare May 3, 2017 15:41
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label May 3, 2017
devversion
devversion approved these changes May 3, 2017
View reviewed changes
Copy link
Member

@devversion devversion left a comment

Choose a reason for hiding this comment

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

Overall LGTM. Thanks

src/lib/button/button.spec.ts

anchorDebugElement = fixture.debugElement.query(By.css('a[md-button]'));
anchorRippleDebugElement = anchorDebugElement.query(By.directive(MdRipple));
anchorRippleInstance = anchorRippleDebugElement.injector.get<MdRipple>(MdRipple);
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need to set the generic type here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Getting directive instances was new to me, so I copied it from other material tests (radios, checkboxes, button-toggles, and sliders all use generic types for injector.get())

Happy to remove if you want

Copy link
Member

Choose a reason for hiding this comment

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

It's alright if other tests do it :)

@devversion devversion added pr: lgtm action: merge The PR is ready for merge by the caretaker labels May 3, 2017
@andrewseguin
Copy link
Contributor

Looks like the tests aren't happy with you, can you see what's up?

@andrewseguin andrewseguin removed the action: merge The PR is ready for merge by the caretaker label May 5, 2017
@willshowell willshowell force-pushed the feat/finish-ripple-disabled-button branch from 038f355 to 56e7c8f Compare May 8, 2017 13:46
@willshowell
Copy link
Contributor Author

@andrewseguin they're passing again after rebase

@willshowell
Copy link
Contributor Author

@jelbourn can pr: merge ready get added back now that tests are passing again?

jelbourn
jelbourn approved these changes May 16, 2017
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 added action: merge The PR is ready for merge by the caretaker presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged labels May 16, 2017
@jelbourn
Copy link
Member

One project has some failing e2e tests with this change, not quite sure why yet

@tinayuangao tinayuangao removed the presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged label May 22, 2017
@tinayuangao tinayuangao merged commit fce2868 into angular:master May 22, 2017
@willshowell willshowell deleted the feat/finish-ripple-disabled-button branch May 24, 2017 15:57
@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.
Reviewers

@devversion devversion devversion approved these changes

@jelbourn jelbourn jelbourn approved these changes

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Retain existing ripples when button state changes to disabled
6 participants
@willshowell @andrewseguin @jelbourn @devversion @googlebot @tinayuangao