fix: `ExpansionTileTheme.shape` assignment in `ExpansionTile` #127749

SirusCodes · 2023-05-27T15:12:24Z

The ExpansionTile was not following shape from ExpansionTileTheme as it was assigning wrong parameter to the tween.

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 · 2023-05-27T15:12:27Z

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.

flutter-dashboard · 2023-06-16T15:15:16Z

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.

TahaTesser · 2023-06-29T13:12:07Z

@SirusCodes
Can you please link an issue this fixes? If doesn't exist please file an issue.
Also looks like there are some failures in this PR.

SirusCodes · 2023-06-29T13:27:04Z

Can you please link an issue this fixes? If doesn't exist please file an issue.

There were no issues related to this when I opened this PR but I guess I can file an issue for it if you say so.

Also looks like there are some failures in this PR.

There is a test failing which was also flawed (I guess) because of the wrong assignment and some extra space character which I mistakenly added.

TahaTesser · 2023-06-29T13:36:23Z

There were no issues related to this when I opened this PR but I guess I can file an issue for it if you say so.

Please do.

TahaTesser

Thanks for fixing this.

Since this is a theme value issue, I took a quick look at the expansion tile theme tests.

It looks like both ExpansionTileTheme - expanded & ExpansionTileTheme - collapsed are testing the shape from the decoration but it's the same shape in both tests. This is the reason this issue wasn't caught earlier.

flutter/packages/flutter/test/material/expansion_tile_theme_test.dart

Line 293 in 7b85956

expect(shapeDecoration.shape, collapsedShape);

flutter/packages/flutter/test/material/expansion_tile_theme_test.dart

Line 211 in 7b85956

expect(shapeDecoration.shape, collapsedShape);

and you updated the theme tests, it is great.

packages/flutter/test/material/expansion_tile_test.dart

SirusCodes · 2023-06-30T13:20:13Z

Hey @TahaTesser reverted the commit that added the test to expansion_tile_test.dart as it was not helping and we don't need to add any test in expansion_tile_theme_test.dart as it already has test for shape which I fixed.

TahaTesser

LGTM

TahaTesser · 2023-06-30T13:21:51Z

This will need a second review from @HansMuller before we land.

HansMuller

LGTM

HansMuller · 2023-06-30T16:25:37Z

packages/flutter/lib/src/material/expansion_tile.dart

Nice catch!

auto-submit · 2023-06-30T16:40:12Z

auto label is removed for flutter/flutter, pr: 127749, due to - The status or check suite Linux flutter_plugins has failed. Please fix the issues identified (or deflake) before re-applying this label.

SirusCodes · 2023-07-01T18:09:23Z

@HansMuller @TahaTesser I cannot understand what is failing in these?

HansMuller · 2023-07-05T15:51:35Z

The customer tests that were failing in the analyzer don't appear to have anything to do with your changes. I've resync'd your PR, hopefully the problem will disappear.

| Analyzing flutter_reactive_ble...
|
|   error • The named parameter 'connectable' is required, but there's no corresponding argument • test/device_connector_test.dart:96:32 • missing_required_argument
|   error • The named parameter 'connectable' is required, but there's no corresponding argument • test/device_scanner_test.dart:24:18 • missing_required_argument
|   error • The named parameter 'connectable' is required, but there's no corresponding argument • test/device_scanner_test.dart:32:18 • missing_required_argument
|   error • The named parameter 'connectable' is required, but there's no corresponding argument • test/reactive_ble_test.dart:234:22 • missing_required_argument
|
| 4 issues found. (ran in 3.6s)
ERROR: One or more tests from flutter_reactive_ble failed.

SirusCodes · 2023-07-07T12:38:51Z

@HansMuller @TahaTesser this test is taking unusually very long to run👀

This reverts commit d06f5132ffc61aefd760475f23d3444b36a03545.

SirusCodes · 2023-07-10T19:33:44Z

@TahaTesser can you merge it 😅👀

…flutter/flutter#127749)

Roll Flutter from 3ec96a8 to 544d30d (66 revisions) flutter/flutter@3ec96a8...544d30d 2023-07-12 [email protected] Roll Packages from 188a846 to 2508714 (14 revisions) (flutter/flutter#130418) 2023-07-12 [email protected] Update `RadioListTile` tests format for M2/M3 (flutter/flutter#130391) 2023-07-12 [email protected] Roll Flutter Engine from d68ea304eeda to 5c887028810d (2 revisions) (flutter/flutter#130413) 2023-07-12 [email protected] Roll Flutter Engine from c7317a58466e to d68ea304eeda (1 revision) (flutter/flutter#130410) 2023-07-12 [email protected] Roll Flutter Engine from 3dbe7dbeb5d4 to c7317a58466e (1 revision) (flutter/flutter#130402) 2023-07-12 [email protected] Roll Flutter Engine from 73093fdd77c3 to 3dbe7dbeb5d4 (1 revision) (flutter/flutter#130399) 2023-07-12 [email protected] Roll Flutter Engine from 29c5c41eeb19 to 73093fdd77c3 (2 revisions) (flutter/flutter#130398) 2023-07-12 [email protected] Roll Flutter Engine from 3482e05776a7 to 29c5c41eeb19 (1 revision) (flutter/flutter#130393) 2023-07-12 [email protected] Roll Flutter Engine from 7de68c62742d to 3482e05776a7 (2 revisions) (flutter/flutter#130390) 2023-07-12 [email protected] Make new issue template for 1P package (flutter/flutter#130065) 2023-07-12 [email protected] Roll Flutter Engine from 1b44d7ee1a60 to 7de68c62742d (4 revisions) (flutter/flutter#130384) 2023-07-12 [email protected] Roll Flutter Engine from e109a3c0c347 to 1b44d7ee1a60 (3 revisions) (flutter/flutter#130380) 2023-07-12 [email protected] Roll Flutter Engine from d00590fde18c to e109a3c0c347 (1 revision) (flutter/flutter#130371) 2023-07-12 [email protected] Roll Flutter Engine from 875d87e02276 to d00590fde18c (5 revisions) (flutter/flutter#130369) 2023-07-12 [email protected] Enable not GCed leak tracking. (flutter/flutter#130159) 2023-07-11 [email protected] Roll Flutter Engine from e2df01610fb3 to 875d87e02276 (3 revisions) (flutter/flutter#130359) 2023-07-11 [email protected] autocomplete: Remove mistaken paragraph in `onSelected` doc (flutter/flutter#130190) 2023-07-11 [email protected] Roll Flutter Engine from 59f234645dd2 to e2df01610fb3 (3 revisions) (flutter/flutter#130357) 2023-07-11 [email protected] Refactor refresh_indicator.1.dart to not use shrinkwrap (flutter/flutter#129377) 2023-07-11 [email protected] Links in `material` library docs are outdated (flutter/flutter#129891) 2023-07-11 [email protected] Roll Flutter Engine from afee1db31e5e to 59f234645dd2 (2 revisions) (flutter/flutter#130352) 2023-07-11 [email protected] Roll Flutter Engine from 75ada1bdf9fd to afee1db31e5e (1 revision) (flutter/flutter#130349) 2023-07-11 [email protected] Upgrade leak_tracker to 7.0.6. (flutter/flutter#130346) 2023-07-11 [email protected] Roll Flutter Engine from 0011db79d41f to 75ada1bdf9fd (2 revisions) (flutter/flutter#130345) 2023-07-11 [email protected] Roll pub packages (flutter/flutter#130289) 2023-07-11 [email protected] `DropdownMenu` can be expanded to its parent size (flutter/flutter#129753) 2023-07-11 [email protected] Roll Packages from 4469c5e to 188a846 (6 revisions) (flutter/flutter#130340) 2023-07-11 [email protected] Roll Flutter Engine from d75c70870f86 to 0011db79d41f (2 revisions) (flutter/flutter#130337) 2023-07-11 [email protected] Roll Flutter Engine from 5e9f0d61a42a to d75c70870f86 (1 revision) (flutter/flutter#130332) 2023-07-11 [email protected] Roll Flutter Engine from 417c50199e14 to 5e9f0d61a42a (1 revision) (flutter/flutter#130330) 2023-07-11 [email protected] Roll Flutter Engine from bfda8f173fea to 417c50199e14 (2 revisions) (flutter/flutter#130324) 2023-07-11 [email protected] Add `Badge` widget to `NavigationBar` and `NavigationRail` examples (flutter/flutter#129834) 2023-07-11 [email protected] Roll Flutter Engine from 2139c8a90822 to bfda8f173fea (2 revisions) (flutter/flutter#130321) 2023-07-11 [email protected] Update labeler for recent changes (flutter/flutter#130168) 2023-07-11 [email protected] fix: `ExpansionTileTheme.shape` assignment in `ExpansionTile` (flutter/flutter#127749) 2023-07-11 [email protected] Roll Flutter Engine from 12aa98177cf2 to 2139c8a90822 (2 revisions) (flutter/flutter#130316) 2023-07-11 [email protected] Roll Flutter Engine from 767f2fb8ab03 to 12aa98177cf2 (1 revision) (flutter/flutter#130315) 2023-07-11 [email protected] Implement preferPaintInterior correctly for _CompoundBorder (flutter/flutter#129851) 2023-07-11 [email protected] Roll Flutter Engine from 153d9e1d598a to 767f2fb8ab03 (1 revision) (flutter/flutter#130313) 2023-07-11 [email protected] Roll Flutter Engine from 2c82dd7ec54b to 153d9e1d598a (2 revisions) (flutter/flutter#130311) 2023-07-11 [email protected] Roll Flutter Engine from 312e4813a880 to 2c82dd7ec54b (1 revision) (flutter/flutter#130309) 2023-07-11 [email protected] Roll Flutter Engine from 95316fbc25a7 to 312e4813a880 (2 revisions) (flutter/flutter#130307) 2023-07-11 [email protected] Add Linux implementation of the platform view example (flutter/flutter#123731) 2023-07-11 [email protected] Roll Flutter Engine from daecd616f5a7 to 95316fbc25a7 (4 revisions) (flutter/flutter#130305) 2023-07-11 [email protected] Roll Flutter Engine from 2a0dd9d2f28e to daecd616f5a7 (3 revisions) (flutter/flutter#130298) 2023-07-10 [email protected] Roll Flutter Engine from 7d054abf842c to 2a0dd9d2f28e (4 revisions) (flutter/flutter#130296) ...

…r#127749) The ExpansionTile was not following `shape` from ExpansionTileTheme as it was assigning wrong parameter to the tween. fixes flutter#129785

flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels May 27, 2023

github-actions bot removed the framework flutter/packages/flutter repository. See also f: labels. label May 27, 2023

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. labels Jun 24, 2023

TahaTesser self-requested a review June 29, 2023 13:11

SirusCodes force-pushed the fix-expansiontile branch from 1575bac to 1c98450 Compare June 29, 2023 18:35

github-actions bot removed f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Jun 29, 2023

TahaTesser changed the title ~~fix: wrong expansion tile shape assignment~~ fix: ExpansionTileTheme.shape assignment in ExpansionTile Jun 30, 2023

TahaTesser requested changes Jun 30, 2023

View reviewed changes

packages/flutter/test/material/expansion_tile_test.dart Outdated Show resolved Hide resolved

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Jun 30, 2023

TahaTesser approved these changes Jun 30, 2023

View reviewed changes

TahaTesser requested a review from HansMuller June 30, 2023 13:21

HansMuller approved these changes Jun 30, 2023

View reviewed changes

packages/flutter/lib/src/material/expansion_tile.dart Outdated

Copy link

Contributor

HansMuller Jun 30, 2023

Choose a reason for hiding this comment

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

Nice catch!

TahaTesser added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 30, 2023

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jun 30, 2023

HansMuller force-pushed the fix-expansiontile branch from 3342c38 to 232a8e6 Compare June 30, 2023 16:59

HansMuller force-pushed the fix-expansiontile branch from 232a8e6 to 062a51a Compare July 5, 2023 15:49

SirusCodes added 4 commits July 10, 2023 12:16

fix: wrong expansion tile shape assignment

af11e98

add test for expansion tile shape inherited from theme

6f0245f

fix breaking ExpansionTileTheme test

b7ac9f1

Revert "add test for expansion tile shape inherited from theme"

e8995e9

This reverts commit d06f5132ffc61aefd760475f23d3444b36a03545.

TahaTesser force-pushed the fix-expansiontile branch from 062a51a to e8995e9 Compare July 10, 2023 10:16

TahaTesser added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 11, 2023

auto-submit bot merged commit af2e282 into flutter:master Jul 11, 2023

engine-flutter-autoroll mentioned this pull request Jul 12, 2023

Roll Flutter from 3ec96a8a4a5d to 21e3bb89676c (65 revisions) flutter/packages#4447

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 12, 2023

af2e282 fix: ExpansionTileTheme.shape assignment in ExpansionTile (…

64ab54d

…flutter/flutter#127749)

engine-flutter-autoroll mentioned this pull request Jul 12, 2023

Roll Flutter from 3ec96a8a4a5d to 544d30dbaddf (66 revisions) flutter/packages#4448

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 12, 2023

af2e282 fix: ExpansionTileTheme.shape assignment in ExpansionTile (…

80e9ddc

…flutter/flutter#127749)

HansMuller mentioned this pull request Jul 21, 2023

Flutter Framework Team: Notable Commits #121415

Closed

fix: ExpansionTileTheme.shape assignment in ExpansionTile #127749

fix: ExpansionTileTheme.shape assignment in ExpansionTile #127749

Uh oh!

Conversation

SirusCodes commented May 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

flutter-dashboard bot commented May 27, 2023

Uh oh!

flutter-dashboard bot commented Jun 16, 2023

Uh oh!

TahaTesser commented Jun 29, 2023

Uh oh!

SirusCodes commented Jun 29, 2023

Uh oh!

TahaTesser commented Jun 29, 2023

Uh oh!

TahaTesser left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SirusCodes commented Jun 30, 2023

Uh oh!

TahaTesser left a comment

Choose a reason for hiding this comment

Uh oh!

TahaTesser commented Jun 30, 2023

Uh oh!

HansMuller left a comment

Choose a reason for hiding this comment

Uh oh!

HansMuller Jun 30, 2023

Choose a reason for hiding this comment

Uh oh!

auto-submit bot commented Jun 30, 2023

Uh oh!

SirusCodes commented Jul 1, 2023

Uh oh!

HansMuller commented Jul 5, 2023

Uh oh!

SirusCodes commented Jul 7, 2023

Uh oh!

SirusCodes commented Jul 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fix: `ExpansionTileTheme.shape` assignment in `ExpansionTile` #127749

fix: `ExpansionTileTheme.shape` assignment in `ExpansionTile` #127749

SirusCodes commented May 27, 2023 •

edited

Loading

TahaTesser left a comment •

edited

Loading