-
Notifications
You must be signed in to change notification settings - Fork 26.9k
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
[Chip] Make sure InkResponse is in the foreground on delete for chips with background color #41463
Conversation
@@ -1808,6 +1808,28 @@ void main() { | |||
expect(find.byType(InkWell), findsOneWidget); | |||
}); | |||
|
|||
testWidgets('Chips should use Ink instead of Container for delete icon ripple', (WidgetTester tester) async { |
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.
Is there a way to test that the ink renders in the foreground without testing the implementation?
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 added a test for this now.
fb6ca90
to
3319984
Compare
3319984
to
14c1e57
Compare
14c1e57
to
300a0ed
Compare
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
…material_chip_bug
741dd65
to
428e1d8
Compare
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 a few small suggestions.
This PR was conflicting with the following PR and thus a test for this branch failed on master: |
… with background color (flutter#41463) * Make sure InkResponse is visible on delete for chips with background color set * Override computeDistanceToActualBaseline in layout builder
…or chips with background color (flutter#41463)" (flutter#43391) This reverts commit 588275e.
Description
For chips with background color set the ripple is not in the foreground, but behind the chip.
Reason for this bug is described here:
https://api.flutter.dev/flutter/material/InkResponse-class.html#the-ink-splashes-arent-visible
Screenshot before:
![before the ink response was behind the chip](https://user-images.githubusercontent.com/1770678/65762586-ad596c80-e121-11e9-9631-55182ff84390.png)
Screenshot after:
![after the ink response is now shown](https://user-images.githubusercontent.com/1770678/65762598-b2b6b700-e121-11e9-8776-531f383148e5.png)
Related Issues
#41461 [Chip] Delete icon ripple not visible for Chip when background color is set
Tests
I added the following tests:
packages/flutter/test/material/chip_test.dart
'Chips should use Ink instead of Container for delete icon ripple'
Checklist
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]
). This will ensure a smooth and quick review process.///
).flutter analyze --flutter-repo
) does not report any problems on my PR.Breaking Change
Does your PR require Flutter developers to manually update their apps to accommodate your change?