Skip to content

Conversation

@DattatreyaReddy
Copy link
Contributor

@DattatreyaReddy DattatreyaReddy commented Aug 3, 2022

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 BoxDecoration to ShapeDecoration

List which issues are fixed by this PR.

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Aug 3, 2022
@flutter-dashboard
Copy link

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.

@bernaferrari
Copy link
Contributor

bernaferrari commented Aug 3, 2022

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.

@DattatreyaReddy
Copy link
Contributor Author

Maybe it is better to use borderShape/borderSide and collapsedBorderShape/collapsedBorderSide? Then other parameters could also be edited. Maybe, just an idea.

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

@bernaferrari
Copy link
Contributor

You can. Most borders lerp between each other. BorderSide also lerps.

@DattatreyaReddy
Copy link
Contributor Author

You can. Most borders lerp between each other. BorderSide also lerps.

Oh, I'll look into it

@DattatreyaReddy
Copy link
Contributor Author

Added Test case for checking Expansion panel Border

@DattatreyaReddy DattatreyaReddy changed the title Adds borderColor, collapsedBorderColor to Expansion Tile Adds border, collapsedBorder properties to Expansion Tile Aug 4, 2022
@DattatreyaReddy DattatreyaReddy force-pushed the expansion_tile_border_color branch from a657449 to 88f7c47 Compare August 4, 2022 12:16
@HansMuller HansMuller requested a review from Piinks August 5, 2022 23:18
Copy link
Contributor

@Piinks Piinks left a 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?

@DattatreyaReddy
Copy link
Contributor Author

DattatreyaReddy commented Aug 17, 2022

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,

  • I don't know why Linux doc test is failing.
    Can you help me with that?

  • Can we consider Border as a property of ExpansionTileThemeData? (Border doesn't fit the context of theme, is what i thought). I'll add it now.

Thank you for your suggestions.

@bernaferrari
Copy link
Contributor

bernaferrari commented Aug 17, 2022

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.

@DattatreyaReddy DattatreyaReddy force-pushed the expansion_tile_border_color branch from 9d5b50d to 0768120 Compare August 17, 2022 06:20
@DattatreyaReddy

This comment was marked as resolved.

@DattatreyaReddy
Copy link
Contributor Author

DattatreyaReddy commented Aug 17, 2022

Clip is not working on the splash of ListTile in expansion tile.

  • press and hold expansion tile after expanding it.

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,
                  ),
                ],
              ),
            ),
          ),
        ),
      ),
    );

@Piinks
Copy link
Contributor

Piinks commented Aug 17, 2022

The docs test is failing because you have a reference in the docs that can't be resolved:

dartdoc:stderr:   error: unresolved doc reference [Theme.divider]
dartdoc:stderr:     from material.ExpansionTile.shape: (file:///b/s/w/ir/x/w/flutter%20sdk/packages/flutter/lib/src/material/expansion_tile.dart:269:22)
dartdoc:stderr:     in documentation inherited from material.ExpansionTile.shape: (file:///b/s/w/ir/x/w/flutter%20sdk/packages/flutter/lib/src/material/expansion_tile.dart:269:22)

@Piinks
Copy link
Contributor

Piinks commented Aug 17, 2022

Clip is not working on the splash of ListTile in expansion tile.

Is this not the expected behavior?

@DattatreyaReddy
Copy link
Contributor Author

DattatreyaReddy commented Aug 18, 2022

Clip is not working on the splash of ListTile in expansion tile.

Is this not the expected behavior?

oh, I thought it should not, as the ListTile has been clipped by the container.

@DattatreyaReddy DattatreyaReddy changed the title Adds border, collapsedBorder properties to Expansion Tile Adds shape, collapsedShape properties to Expansion Tile Aug 23, 2022
@DattatreyaReddy DattatreyaReddy force-pushed the expansion_tile_border_color branch from 7236bc4 to bab2509 Compare August 25, 2022 07:34
Copy link
Contributor

@Piinks Piinks left a 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.

Copy link
Contributor

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?

Copy link
Contributor Author

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

@DattatreyaReddy
Copy link
Contributor Author

DattatreyaReddy commented Sep 15, 2022

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.

  • Previously (this is the default and the user cannot edit it (for example the border shape or color))

image

image

  • With this PR (the user can edit the shape and color of the borders )

image

image

@Piinks
Copy link
Contributor

Piinks commented Sep 16, 2022

It looks like CI is failing, can you rebase with the latest on master?

@christopherfujino
Copy link
Contributor

This PR looks like it was branched off of a different branch from master, resulting in a large diff and meaning it will not be mergeable. Please see tree hygiene for more details about how to open pull requests to Flutter repositories.

@christopherfujino christopherfujino removed their request for review September 17, 2022 00:26
@christopherfujino christopherfujino removed the request for review from keyonghan September 17, 2022 00:26
@DattatreyaReddy
Copy link
Contributor Author

This PR looks like it was branched off of a different branch from master, resulting in a large diff and meaning it will not be mergeable. Please see tree hygiene for more details about how to open pull requests to Flutter repositories.

Should I force undo the merge commit and rebase with git fetch upstream; git rebase upstream/master; git push origin your_branch_name ?

@DattatreyaReddy DattatreyaReddy force-pushed the expansion_tile_border_color branch from 31d6369 to 1e00266 Compare September 17, 2022 00:39
@DattatreyaReddy DattatreyaReddy force-pushed the expansion_tile_border_color branch from 1e00266 to 0161731 Compare September 17, 2022 00:56
@DattatreyaReddy
Copy link
Contributor Author

sorry but I think I messed up. I lost all my commits is there any way to undo it?

@bernaferrari
Copy link
Contributor

git reflog is your friend

@DattatreyaReddy DattatreyaReddy force-pushed the expansion_tile_border_color branch 2 times, most recently from 8c97095 to 66c7936 Compare September 19, 2022 05:41
@DattatreyaReddy
Copy link
Contributor Author

I wasn't able to rebase the repo, when I tried I'm getting a suggestion to push 24 changes as shown bellow.

image

image

@DattatreyaReddy
Copy link
Contributor Author

Should I open another PR?

@Piinks
Copy link
Contributor

Piinks commented Sep 22, 2022

You can open a new PR, but git reflog is also pretty helpful if you want to rollback to an earlier state.

@DattatreyaReddy
Copy link
Contributor Author

I've rolled back to previous commit. This is happening after that.

@Piinks
Copy link
Contributor

Piinks commented Sep 22, 2022

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! :)

@bernaferrari
Copy link
Contributor

bernaferrari commented Sep 22, 2022

you click in your github flutter fork to sync changes
you checkout, then pull master (fetch + reset --hard works)
you checkout your branch, you rebase this branch on top of master (right click on origin/master, rebase thisBranch)
you force push (git push --force)

if git is too hard, you can use IntelliJ, its git features are really good.

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

Labels

f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Control border in ExpansionTile when expand children. Proposal to add decoration / border argument to ExpansionTile to control borders

4 participants