-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
feat(ripple): no longer require additional setup when using MatRipple directive #11913
Conversation
There was a problem hiding this 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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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
Caretaker note: be on the lookout for failures where people are overriding |
Hi @devversion! This PR has merge conflicts due to recent upstream merges. |
c5ca1cd
to
10affcf
Compare
… 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> ```
10affcf
to
423934c
Compare
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Currently if a developer wants to use the
matRipple
directive, he needs to set the position of the element to eitherabsolute
orrelative
.This makes the
matRipple
directive kind of not intuitive. Developers should be just able to place thematRipple
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 elementReferences #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.