-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
fix(material/core): ripple coords when trigger is not the container #29020
Open
enten
wants to merge
1
commit into
angular:main
Choose a base branch
from
enten:ripple-coords-when-trigger-is-not-the-container
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This assumes that the dimensions of the trigger are translatable to the container right? I wonder if we would rather want to prevent the case where the trigger is outside of the container? What do you think?
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.
That's right. And I don't see any case where the dimensions of the trigger aren't translatable to the container because here we do simple proportion calculations (except when the container height or width equal to 0 but this case is already covered).
No, I don't want to prevent the case where the trigger is outside of the container. I want to translate a position clicked in trigger element into the ripple container.
In this stackblitz to reproduce the current behavior, I apply
[matRippleRadius]="10"
on the ripple container. But if you don't apply a radius, you can see that the ripple element coords is outside the ripple container (I clicked where there is the letter "X" in red) and create a huge radius:In this example, the experience is not too shocking because the trigger is near to the ripple container. But in case where the trigger is far from the ripple container: the ripple element rendering is strange (when we think about user experience). It's a better user experience when the ripple element coords is inside the ripple container in coords translated from the original coords (where the user clicked on the trigger).
Note: indeed, this translation has no effect when the ripple element is centered.
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.
Right, but does it make sense to do a simple proportion calculation? what's the use-case? I.e. in most cases the ripple should originate where the click happens, and different proportions wouldn't necessarily make sense then, right?
What's the use-case though? Why wouldn't the trigger not be inside the container? or a sibling that is "positioned over the ripple container"?
Yeah, this is a bug. Good catch, and one solution would be to disallow the trigger to be outside of the boundaries of the container I think.
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 don't think that is a bug. As we can read in ripple.md: it's a feature "to show ripples on interaction with some other element" that the container.
Check yourself: I made another stackblitz and compare the user experience with the both triggers (I think that a demonstration can be more explicit than words).
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.
yeah, I actually created the implementation of ripple a couple of years ago. The intent was to support triggers of different elements, but not necessarily disconnected from the container visually (but to support the trigger overlaying the ripple elements, to e.g. not prevent further clicks).
We can limit this feature, or clarify the intent to address this problem. It's just an option we can consider. We can certainly also fix this if it makes sense to do so.
I follow what you are trying to achieve, but what is the real-world use-case here? Why would you want to display the ripple in a totally different element? Ripples from the Material specification generally don't seem to mention such a scenario: https://m2.material.io/develop/ios/supporting/ripple
Also cc. @crisbeto / @andrewseguin
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.
It's subjective. The first time I use a trigger element different than the ripple container element (where the trigger element is far from the ripple container element): I don't understand why the ripple effect seems to be strange (the ripple effect was too different than the case when the ripple container element is also the trigger element). I needed to investigate to understand what causes this difference.
After that, I wrote a code to adapt the ripple position like in this stackblitz. And later I made this pull request.
I would have been curious to have other opinions than ours. But if you want we can close this topic.