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 InkWell ripple visible on right click when not expected #124386
Fix InkWell ripple visible on right click when not expected #124386
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.
This LGTM thanks for fixing this!
In regards to not having the splash show when a pop up menu is activated by the secondary button I think I would still expect some visual indication that the button has been tapped. I don't see a reason why this should not be customizable though.
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 with one question.
I'm not sure if there is anything in the spec about this, but not showing the ripple when there is no secondary handler sounds like the right thing to do to me 👍
bool get primaryButtonEnabled => _primaryButtonEnabled(widget); | ||
bool get secondaryButtonEnabled => _secondaryButtonEnabled(widget); |
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.
What's the reason for making these public? I'm not against it I just want to make sure we don't expose new APIs that could be private.
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 think there is no good reason and that these accessors should be private, good catch!
I have updated the PR accordingly.
d9455ea
to
7c4ac9d
Compare
…124386) Right clicking an InkWell with no secondary tap handlers will no longer cause a ripple.
Description
This PR removes the splash effect when no secondary button callbacks are defined.
InkWell
defines several useful callbacks related to the secondary button.When no secondary button callbacks are defined the splash effect should not be visible.
Implementation choice
This PR does not remove the splash effect when at least one of the secondary button callbacks is defined (this behavior was proposed in #119058 (review)).
Maybe we should consider if showing a splash effect for the secondary button is an expected behavior (and decide to remove it or make it configurable) ? A possible use case is an InkWell which defines both primary button callbacks and secondary button callbacks, where the primary button is used to execute an action and the secondary button to show a popup menu, in this case, it might be a better UX choice to not show the splash effect for the secondary button.
@Renzo-Olivares @justinmc
Related Issue
Fixes #124328
Tests
Adds 1 test.