-
Notifications
You must be signed in to change notification settings - Fork 26.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
_SaltedKey solution to ExpansionPanelList
#11902
Conversation
@@ -70,5 +70,52 @@ void main() { | |||
expect(find.text('B'), findsOneWidget); | |||
box = tester.renderObject(find.byType(ExpansionPanelList)); | |||
expect(box.size.height - oldHeight, greaterThanOrEqualTo(100.0)); // 100 + some margin | |||
|
|||
// multiple panel list test |
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.
you should put this in a separate testWidgets call, with this comment as the title of the test
home: new ListView( | ||
children: <ExpansionPanelList>[ | ||
new ExpansionPanelList( | ||
expansionCallback: (int _index, bool _isExpanded){ |
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.
trivial nit: space before {
new ExpansionPanelList( | ||
expansionCallback: (int _index, bool _isExpanded){ | ||
index = _index; | ||
isExpanded = _isExpanded; |
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.
since you don't actually interact with this widget, you can probably just leave the callback empty (or maybe omit it altogether?)
headerBuilder: (BuildContext context, bool isExpanded) { | ||
return new Text(isExpanded ? 'B': 'A'); | ||
}, | ||
body: const SizedBox(height:100.0), |
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.
minor style nit: space after :
body: const SizedBox(height:100.0), | ||
isExpanded: true, | ||
), | ||
], |
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.
same comments on this side
|
||
expect(find.text('A'), findsNothing); | ||
expect(find.text('B'), findsOneWidget); | ||
// Multiple panel lists scenario only fails when expanded |
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.
no need for this comment
That test will be fine, just some minor tweaks needed before we land it. Thanks! |
@Skylled Thanks for your contribution, I'm going to land this as-is and if there's any more style nits we'll fix them in post. |
_SaltedKey implementation courtesy of @Hixie
Tested and confirmed working. Screenshot
Fixes #11166