Skip to content
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

Merged
merged 3 commits into from
Sep 26, 2017
Merged

_SaltedKey solution to ExpansionPanelList #11902

merged 3 commits into from
Sep 26, 2017

Conversation

Skylled
Copy link
Contributor

@Skylled Skylled commented Sep 1, 2017

_SaltedKey implementation courtesy of @Hixie
Tested and confirmed working. Screenshot
Fixes #11166

_SaltedKey implementation courtesy of @Hixie
Tested and confirmed working.
Fixes #11166
@@ -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
Copy link
Contributor

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){
Copy link
Contributor

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;
Copy link
Contributor

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),
Copy link
Contributor

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,
),
],
Copy link
Contributor

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
Copy link
Contributor

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

@Hixie
Copy link
Contributor

Hixie commented Sep 6, 2017

That test will be fine, just some minor tweaks needed before we land it. Thanks!

@Hixie
Copy link
Contributor

Hixie commented Sep 26, 2017

@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.

@Hixie Hixie merged commit 7a35db1 into flutter:master Sep 26, 2017
@Skylled Skylled deleted the patch-1 branch October 8, 2017 13:18
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expansion Panels in Tab View break due to global keys
3 participants