ExpansionTile children background color #158421
Conversation
| child: Column( | ||
| crossAxisAlignment: widget.expandedCrossAxisAlignment ?? CrossAxisAlignment.center, | ||
| children: widget.children, | ||
| child: Material( |
There was a problem hiding this comment.
I think we can change Material to ColoredBox.
There was a problem hiding this comment.
I think this is extra. Why do we need this?
There was a problem hiding this comment.
please see this comment
There was a problem hiding this comment.
Sorry, but it's still unclear to me why we want to isolate the children from the padding color. I thought the purpose of this PR was to change the children's background color and make them different from the header background color?
There was a problem hiding this comment.
As mentioned here, our proposal is to change the padding color in children.
There was a problem hiding this comment.
For that example, when we specify the ListTile.tileColor, it looks like we only need a way to customize the padding color. But if we remove the tileColor assignment, I think it would be better if the children background color is the same as the "padding" color.
As for the property name, I think the original childrenBackgroundColor is better than the current padding color. Please let me know what you think:)
There was a problem hiding this comment.
childrenBackgroundColor gives the impression that it will change the children's background color. However, we need to set a children's padding color separately when it differs from backgroundColor. Therefore, I believe childrenPaddingColor would be a more appropriate name.
There was a problem hiding this comment.
childrenBackgroundColor gives the impression that it will change the children's background color.
I think this is what we are trying to fix (as the issue mentioned). Just feel it's a bit odd to only add a color for padding. My proposal would be: we add a childrenBackgroundColor, and at the same time, we remove the Material here because it's not necessary to wrap children with a Material which provides a different color (unless there's a spec indicating we should do so).
In this way, no matter whether we have paddings or not, the children background color can be customized by the new property.
There was a problem hiding this comment.
What if the user needs different colors for padding and background? Currently, the user can set the children's color, but there's no way to differentiate between childrenPaddingColor and backgroundColor. That limitation is the main issue.
There was a problem hiding this comment.
The issue we are trying to fix is to make the children's background color customizable. If so, then the change without adding Material is sufficient. I don't think it's a good idea to add a property that only customizes the padding color (please let me know if there's such an example in the material library). This widget is Material 1 component which is pretty old and the specs also don't mention this particular use case.
What if the user needs different colors for padding and background?
There's always a way to differentiate between children's padding and children's background color. children is just a list of widgets so we can put any widget with different background colors there, right?
|
Hey @Halim-Elmenshawi would you like to continue working on this and address the above feedback? |
Yes, for sure! However, I am currently on vacation. I will be back in two weeks, and I will take care of it again. |
|
Thanks for the update! I am going to mark this as a draft in the meantime, please mark it ready for review when it is! Thanks again for contributing. :) |
|
Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change). If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
Done |
…ub.com/Halim-Elmenshawi/flutter into ExpansionTile_children_backgroundColor
| child: Column( | ||
| crossAxisAlignment: widget.expandedCrossAxisAlignment ?? CrossAxisAlignment.center, | ||
| children: widget.children, | ||
| child: Material( |
There was a problem hiding this comment.
I think this is extra. Why do we need this?
Co-authored-by: Qun Cheng <36861262+QuncCccccc@users.noreply.github.com>
|
Golden file changes are available for triage from new commit, Click here to view. For more guidance, visit Writing a golden file test for Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
| /// The color to display behind the expanded children section of the tile. | ||
| /// | ||
| /// If this property is null, the value from [ExpansionTileThemeData.childrenPaddingColor] is | ||
| /// used. If both are null, then [backgroundColor] will be used. |
There was a problem hiding this comment.
It's unclear what 'both' is referring to here. Maybe a better term would be 'If that is also null', where 'that' can be directly inferred as the theme color.
| @@ -407,6 +408,12 @@ class ExpansionTile extends StatefulWidget { | |||
| /// [ExpansionTileThemeData]. | |||
| final EdgeInsetsGeometry? childrenPadding; | |||
|
|
|||
| /// The color to display behind the expanded children section of the tile. | |||
There was a problem hiding this comment.
This will need more of a description of the desired use case.
| expect(childrenPaddingColor, Colors.amber); | ||
| }); | ||
|
|
||
| testWidgets('ExpansionTile without using childrenPaddingColor Color', ( |
There was a problem hiding this comment.
Isn't this test just a duplicate of the test for backgroundColor? Is the test needed?
|
Curious about your thoughts on this comment: #158421 (comment) I still think adding background color instead of padding color makes more sense:) |
|
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 Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
Description:
This PR addresses Issue #158419 related to the ExpansionTile widget.
Add
childrenBackgroundColorProperty toExpansionTileWidget for Customizing Expanded Children BackgroundSolution
This PR introduces a new property,
childrenBackgroundColor, to theExpansionTilewidget andExpansionTileThemeclass . This new property allows developers to define a specific background color for the expanded children section, independent of the backgroundColor applied to the header.Fixes: #158419