-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Adds shape, collapsedShape properties to Expansion Tile #108861
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
Adds shape, collapsedShape properties to Expansion Tile #108861
Conversation
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
Maybe it is better to use borderShape/borderSide and collapsedBorderShape/collapsedBorderSide? Then other parameters could also be edited. Maybe, just an idea. Also, your PR is missing tests. |
We could do that, But the problem is the animation. If we are specifying the border parameter then we can't run animation on it |
|
You can. Most borders lerp between each other. BorderSide also lerps. |
Oh, I'll look into it |
|
Added Test case for checking Expansion panel Border |
a657449 to
88f7c47
Compare
Piinks
left a comment
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.
It looks like some tests are failing, can you take a look?
Also, why not add this to the theme as well like the other properties?
Hi @Piinks,
Thank you for your suggestions. |
|
It is failing because the PR needs to be rebased on top of the last flutter master (it is using a too old version for the CI). You rebase, then force push. |
9d5b50d to
0768120
Compare
This comment was marked as resolved.
This comment was marked as resolved.
|
Clip is not working on the splash of ListTile in expansion tile.
import 'package:flutter/material.dart';
import 'package:flutter/widgets.dart';
void main() => runApp(
MaterialApp(
home: Scaffold(
body: Padding(
padding: const EdgeInsets.all(64.0),
child: Center(
child: ExpansionTile(
backgroundColor: Colors.red,
collapsedBackgroundColor: Colors.green,
shape: CircleBorder(
side: BorderSide(),
// borderRadius: BorderRadius.circular(300),
),
title: Text(
'Hello, world!',
key: Key('title'),
textDirection: TextDirection.ltr,
),
children: [
Text(
'Hello, world!',
textDirection: TextDirection.ltr,
),
Text(
'Hello, world!',
textDirection: TextDirection.ltr,
),
Text(
'Hello, world!',
textDirection: TextDirection.ltr,
),
Text(
'Hello, world!',
textDirection: TextDirection.ltr,
),
],
),
),
),
),
),
);
|
|
The docs test is failing because you have a reference in the docs that can't be resolved: |
Is this not the expected behavior? |
oh, I thought it should not, as the ListTile has been clipped by the container. |
7236bc4 to
bab2509
Compare
Piinks
left a comment
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 can't quite tell what this is achieving versus the original default. Can you add an image before/after your change of the default tile? I think this might be a breaking change, so I am running it through additional tests to check.
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.
Why are all of these casts necessary? Is there a way to avoid them?
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.
As expandedContainerDecoration.shape is a ShapeBorder we need to cast it to Border to get the top, and bottom colors for the test.
Mainly cause expandedContainerDecoration is ShapeDecoration
|
|
It looks like CI is failing, can you rebase with the latest on master? |
|
This PR looks like it was branched off of a different branch from |
Should I force undo the merge commit and rebase with |
31d6369 to
1e00266
Compare
1e00266 to
0161731
Compare
|
sorry but I think I messed up. I lost all my commits is there any way to undo it? |
|
|
8c97095 to
66c7936
Compare
|
Should I open another PR? |
|
You can open a new PR, but |
|
I've rolled back to previous commit. This is happening after that. |
|
Have you tried rebasing again after rolling back? I can't be of much help troubleshooting git unfortunately. If opening a new PR is easier, that's ok to do! :) |
|
you click in your github flutter fork to sync changes if git is too hard, you can use IntelliJ, its git features are really good. |








Adds [
borderColor, collapsedBorderColor,border, collapsedBorder] shape, collapsedShape properties to Expansion Tile widget.uses ShapeBorderTween to add expand/close animation
replaces _borderColorTween with _borderTween
Changes Container decoration of expansionTile from
BoxDecorationtoShapeDecorationList which issues are fixed by this PR.
Pre-launch Checklist
///).