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(ripple): no longer require additional setup when using MatRipple directive #11913

Merged

Conversation

devversion
Copy link
Member

Currently if a developer wants to use the matRipple directive, he needs to set the position of the element to either absolute or relative.

This makes the matRipple directive kind of not intuitive. Developers should be just able to place the matRipple directive, and everything works.

Developers should not ask themselves at some point: Why do my ripples don't display at all? ... Ahh I need to apply position: relative to the host element


<button matRipple>Button with ripple</button>

<mat-toolbar matRipple>Toolbar with ripple</button>

<mat-card matRipple>Card with ripple</mat-card>

References #7543.

@jelbourn Not sure if this should be either minor or major. Technically the increase of specificity can break people. Although there should be nobody that overwrites the ripple containers.

@devversion devversion added the target: minor This PR is targeted for the next minor release label Jun 24, 2018
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jun 24, 2018
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.

Code-wise LGTM, but I think we should evaluate whether this is a common enough use-case to warrant the increased specificity.

.mat-button-toggle-ripple {
// Increase specificity because ripple styles are part of the `mat-core` mixin and can
// potentially overwrite the absolute position of the container.
.mat-button-toggle .mat-button-toggle-ripple {
Copy link
Member

Choose a reason for hiding this comment

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

Consider nesting this under the .mat-button-toggle selector. Same goes for the other changed child selectors in this PR.

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 considered doing that, but went with that approach because I wanted to separate the ripple stuff as much as I can from the actual component styles.

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

There's about 89 instances of people overriding mat-something-ripple in Google, so we'll see if it causes any issues

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Jun 26, 2018
@jelbourn
Copy link
Member

Caretaker note: be on the lookout for failures where people are overriding mat-something-ripple

@ngbot
Copy link

ngbot bot commented Jul 11, 2018

Hi @devversion! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

@devversion devversion force-pushed the feat/ripple-no-user-css-required branch from c5ca1cd to 10affcf Compare July 11, 2018 16:05
@devversion devversion requested a review from amcdnl as a code owner July 11, 2018 16:05
… directive

Currently if a developer wants to use the `matRipple` directive, he needs to set the position of the element to either `absolute` or `relative`. This makes the `matRipple` directive kind of not intuitive. Developers should be just able to place the `matRipple` directive, and everything works.

Developers should not ask themselves at some point: _Why do my ripples don't display at all? ... Ahh I need to apply `position: relative` to the host element_

```html
<button matRipple>Button with ripple</button>
<mat-toolbar matRipple>Toolbar with ripple</button>
<mat-card matRipple>Card with ripple</mat-card>
```
@devversion devversion force-pushed the feat/ripple-no-user-css-required branch from 10affcf to 423934c Compare July 20, 2018 15:22
@josephperrott josephperrott merged commit d796776 into angular:master Jul 20, 2018
@devversion devversion deleted the feat/ripple-no-user-css-required branch July 20, 2018 18:25
@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 9, 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 merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants