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.

Sign up for GitHub

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

Jump to bottom

Added expandedColor and color property to ExpansionPanelList arrow icon. #96705

Closed
wants to merge 4 commits into from
Closed

Added expandedColor and color property to ExpansionPanelList arrow icon. #96705

wants to merge 4 commits into from

Conversation

dheerajv09
Copy link
Contributor

Adds expandedColor and color property to ExpansionPanelList expanded icon.

fixes #95011

@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Jan 15, 2022
werainkhatri
werainkhatri requested changes Jan 16, 2022
View reviewed changes
Copy link
Member

@werainkhatri werainkhatri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for your contribution.

I just have a few nits and suggestions.

packages/flutter/lib/src/material/expansion_panel.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/expansion_panel.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/expansion_panel.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/expansion_panel.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/expansion_panel.dart Outdated Show resolved Hide resolved
packages/flutter/test/material/expansion_panel_test.dart Outdated Show resolved Hide resolved
packages/flutter/test/material/expansion_panel_test.dart Outdated Show resolved Hide resolved
packages/flutter/test/material/expansion_panel_test.dart Outdated Show resolved Hide resolved
packages/flutter/test/material/expansion_panel_test.dart Outdated Show resolved Hide resolved
packages/flutter/test/material/expansion_panel_test.dart Outdated Show resolved Hide resolved
@HansMuller HansMuller self-requested a review January 21, 2022 22:46
werainkhatri
werainkhatri requested changes Jan 23, 2022
View reviewed changes
Copy link
Member

@werainkhatri werainkhatri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay. Minor nits and doc suggestions.

packages/flutter/test/material/expansion_panel_test.dart Outdated Show resolved Hide resolved
packages/flutter/test/material/expansion_panel_test.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/expansion_panel.dart Outdated Show resolved Hide resolved
packages/flutter/lib/src/material/expansion_panel.dart Outdated Show resolved Hide resolved
werainkhatri
werainkhatri approved these changes Jan 23, 2022
View reviewed changes
Copy link
Member

@werainkhatri werainkhatri left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

You'll need an LGTM from another member too, I suppose that'll be @HansMuller.

goderbauer
goderbauer reviewed Apr 5, 2022
View reviewed changes
packages/flutter/lib/src/material/expansion_panel.dart Outdated
Comment on lines 267 to 269
/// Defaults to [Colors.black54] when the theme's
/// [ThemeData.brightness] is [Brightness.light] and to
/// [Colors.white] when it is [Brightness.dark].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this code is just copied from https://master-api.flutter.dev/flutter/material/ExpandIcon/expandedColor.html? Maybe use a macro so we don't forget to update it here should it ever change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Maybe use a macro so we don't forget to update it here should it ever change."
Hi, I didn't get it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@goderbauer any updates??

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://pub.dev/packages/dartdoc#macros - look for examples of this in our docs.

packages/flutter/lib/src/material/expansion_panel.dart Outdated
Comment on lines 260 to 262
/// Defaults to [Colors.black54] when the theme's
/// [ThemeData.brightness] is [Brightness.light] and to
/// [Colors.white60] when it is [Brightness.dark].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as below.

@dheerajv09
Copy link
Contributor Author

Hi @goderbauer , can you tell what's all the failed check error means??.
I think there is also problem , the way I have used macros.(what's wrong)

@goderbauer
Copy link
Member

There are problems reported for the newly added doc macros:

dartdoc:stdout: Generating docs for library material from package:flutter/material.dart...
dartdoc:stderr:   error: undefined macro [flutter.material.ExpandIcon.color]
dartdoc:stderr:     from material.ExpansionPanelList.expandedIconColor: (file:///tmp/flutter%20sdk/packages/flutter/lib/src/material/expansion_panel.dart:258:16)
dartdoc:stderr:   error: undefined macro [flutter.material.ExpandIcon.color]
dartdoc:stderr:     from material.ExpansionPanelList.iconColor: (file:///tmp/flutter%20sdk/packages/flutter/lib/src/material/expansion_panel.dart:253:16)

Furthermore, you probably have to merge in the latest master to resolve the other test failures.

goderbauer
goderbauer reviewed May 24, 2022
View reviewed changes
packages/flutter/lib/src/material/expansion_panel.dart
@@ -58,7 +58,7 @@ typedef ExpansionPanelHeaderBuilder = Widget Function(BuildContext context, bool
/// expanded or collapsed. The body of the panel is only visible when it is
/// expanded.
///
/// {@youtube 560 315 https://www.youtube.com/watch?v=2aJZzRMziJc}
///{@youtube 560 315 https://www.youtube.com/watch?v=2aJZzRMziJc}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please leave the space here.

packages/flutter/lib/src/material/expansion_panel.dart
@@ -243,6 +247,16 @@ class ExpansionPanelList extends StatefulWidget {
/// is null, then [ThemeData.dividerColor] is used.
final Color? dividerColor;

/// The color of the arrow icon when the panel is collapsed.
///
/// {@macro flutter.material.ExpandIcon.color}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like there is no template defined for this macro? That's why the doc check is failing.

@flutter-dashboard
Copy link

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

For more guidance, visit Writing a golden file test for package:flutter.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@HansMuller
Copy link
Contributor

This PR appears to have gone stale, comments from late May haven't been addressed. I'm going to close it now; if you decide to resume work feel free to reopen it.

@HansMuller HansMuller closed this Aug 31, 2022
@M97Chahboun
Copy link
Contributor

@dheerajv09
Are you will resume it or no, because I need it,
if you are busy let me open new PR,
Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@goderbauer goderbauer goderbauer left review comments

@SuhwanCha SuhwanCha SuhwanCha left review comments

@werainkhatri werainkhatri werainkhatri approved these changes

@HansMuller HansMuller Awaiting requested review from HansMuller

Assignees
No one assigned
Labels
f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Proposal] add property to change ExpansionPanelList arrow icon color
6 participants
@dheerajv09 @goderbauer @HansMuller @M97Chahboun @SuhwanCha @werainkhatri