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

feat(select): allow disabling ripples for options #5967

Merged
merged 1 commit into from
Jul 28, 2017

Conversation

devversion
Copy link
Member

  • Adds a binding that allows developers to disable ripples for every option in a select.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jul 22, 2017
@@ -99,17 +92,22 @@ export class MdOption extends _MdOptionMixinBase implements CanDisableRipple {
get disabled() { return (this.group && this.group.disabled) || this._disabled; }
set disabled(value: any) { this._disabled = coerceBooleanProperty(value); }

/** Whether ripples for the option are disabled. */
@Input()
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that this should be an Input. The only way to disable ripples on options should be programmatic. It also means that we won't have to coerce the value.

Copy link
Member Author

Choose a reason for hiding this comment

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

The ability to disable the ripples from a binding has been already added in addf1ce. I don't really see anything that would go against just providing this flexibility.

Copy link
Member

Choose a reason for hiding this comment

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

I think that there might be some overhead to having to maintain the input. Also there's the extra JS being generated because of the getter and setter, although we can't really avoid those because it has to markForCheck.

Copy link
Member Author

Choose a reason for hiding this comment

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

It might be true that it might not happen that someone just disables ripples for a single option.

From the payload size perspective it shouldn't make any difference, so it's more a question of API design.

I'm fine removing the @Input here. Also since addf1ce is not in master that long, I assume it won't be a breaking change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated the tests. Please have another look.

expect(overlayContainerElement.querySelectorAll('.mat-ripple-element').length)
.toBe(0, 'Expected no ripples to show up initially.');

dispatchFakeEvent(options[0], 'mousedown');
Copy link
Member

@crisbeto crisbeto Jul 22, 2017

Choose a reason for hiding this comment

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

I'm not sure whether we care about the DOM events that are happening in these tests since this is tested well enough in the ripple tests. Looping through all the options and verifying that the flag is correct should be enough. Also it could use a test where it adds a new option and checks that it got the proper disableRipple value.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure about that. This test would just ensure that the disableRipple property changed properly and also got reflected properly in the view. Verifying for example that markForCheck is placed properly.

I agree on adding a test for the property itself though.

Copy link
Member

Choose a reason for hiding this comment

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

My issue with this approach is that it's testing functionality that it's not responsible for. That functionality already has tests of its own.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's an argument. I will change it.

Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

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

LGTM

fixture.componentInstance.disableRipple = true;
fixture.detectChanges();

expect(options.every(option => option.disableRipple === true))
Copy link
Member

Choose a reason for hiding this comment

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

Can be reduced to option => option.disableRipple.

Copy link
Member Author

Choose a reason for hiding this comment

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

True. But I want to make it clear that we are checking for a truthy value here.

@crisbeto crisbeto added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Jul 22, 2017
@kara kara removed their assignment Jul 24, 2017
@andrewseguin
Copy link
Contributor

Rebase please

* Adds a binding that allows developers to disable ripples for every option in a select.
@devversion
Copy link
Member Author

@andrewseguin Done.

@andrewseguin andrewseguin merged commit 34ec068 into angular:master Jul 28, 2017
@devversion devversion deleted the feat/select-disable-ripples branch November 11, 2017 10:22
@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 7, 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.

5 participants