Fix ExpandIcon color when ExpansionPanel.canTapOnHeader true (#147097) #147098

benji-farquhar · 2024-04-20T01:31:09Z

Fixes #147097. The expand/collapse icon should not be the disabled color when ExpansionPanel.canTapOnHeader is true because the ExpansionPanel is active, and we may want to keep the icon there as a visual indicator to the user that it is expandable/collapsable.

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.
I followed the breaking change policy and added Data Driven Fixes where supported.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

flutter-dashboard · 2024-04-20T01:31:12Z

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 "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use 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.

google-cla · 2024-04-20T01:31:13Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

benji-farquhar · 2024-05-08T07:23:28Z

@TahaTesser trailing space is fixed. Not sure why some tests (see test commands below) failed in the remote build, as they pass when I run them locally with:

flutter test material/app_test.dart                      
flutter test widgets/draggable_scrollable_sheet_test.dart
flutter test painting/star_border_test.dart

Any idea why they failed in this build?

TahaTesser · 2024-05-08T09:43:39Z

There seems to be a bug on master which doesn't update code so i couldn't test your change. I can confirm it works now.

flutter test material/app_test.dart
flutter test widgets/draggable_scrollable_sheet_test.dart
flutter test painting/star_border_test.dart

These test files are unrelated to ExpansionPanel widget. Try running test on expansion_panel_test.dart file. There is a failure in "'Panel header has semantics, canTapOnHeader = true'" test.

I've updated your fix to mitigate the changing semantics and avoid adding ignorePointer when not necessary.

We can assign widget.expandIconColor to disabledColor in the ExpandIcon widget when canTapOnHeader is true. This allows ExpandIcon to show correct color and avoid test failures.

        child: ExpandIcon(
          color: widget.expandIconColor,
          disabledColor: child.canTapOnHeader ? widget.expandIconColor : null,
          isExpanded: _isChildExpanded(index),
          padding: _kExpandIconPadding,
          onPressed: !child.canTapOnHeader
              ? (bool isExpanded) => _handlePressed(isExpanded, index)
              : null,
        ),

If you still need help testing the changes. let me know. This requires checking icon color when using canTapOnHeader prop. You can look into existing tests for ideas on how to test this.

benji-farquhar · 2024-05-09T07:22:30Z

@TahaTesser Thanks. I have implemented the changes with tests. Let me know if it requires any changes.

packages/flutter/test/material/expansion_panel_test.dart

TahaTesser

LGTM!

bleroux

LGTM!

auto-submit · 2024-05-09T14:31:35Z

auto label is removed for flutter/flutter/147098, due to - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.

…(#147097) (flutter/flutter#147098)

flutter/flutter@00f4066...2bfb1b0 2024-05-09 goderbauer@google.com Bump flutter_lints to 4.0 (flutter/flutter#148020) 2024-05-09 104349824+huycozy@users.noreply.github.com Add Badge example (flutter/flutter#148053) 2024-05-09 farquhar.benji@gmail.com Fix ExpandIcon color when ExpansionPanel.canTapOnHeader true (#147097) (flutter/flutter#147098) 2024-05-09 engine-flutter-autoroll@skia.org Roll Flutter Engine from c2e874fdc164 to c0fd3386d018 (1 revision) (flutter/flutter#148040) 2024-05-09 engine-flutter-autoroll@skia.org Roll Flutter Engine from fe08238f5a52 to c2e874fdc164 (1 revision) (flutter/flutter#148038) 2024-05-09 engine-flutter-autoroll@skia.org Roll Flutter Engine from 69f2d9610a18 to fe08238f5a52 (1 revision) (flutter/flutter#148034) 2024-05-09 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#148011) 2024-05-09 engine-flutter-autoroll@skia.org Roll Flutter Engine from bd58d2b2d655 to 69f2d9610a18 (7 revisions) (flutter/flutter#148030) 2024-05-09 kevmoo@users.noreply.github.com [web] Update wasm CLI details to be clear JavaScript is ALSO compiled (flutter/flutter#147944) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC bmparr@google.com,rmistry@google.com,stuartmorgan@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

Relates to #147098 and #147097 Aside from fixing the expland/collapse icon color in the other PR, I noticed the splash color on both the icon button and the full expansion panel (if ExpansionPanel.canTapOnHeader is set to true) is just `Theme.of(context).splashColor` on the `InkWell` and `Theme.of(context).highlightColor` on the `IconButton` which may not suit the color scheme of the `ExpansionPanel`, so I have added a custom field `splashColor`, which will effect both the `IconButton` and the full panel `Inkwell`.

Relates to flutter#147098 and flutter#147097 Aside from fixing the expland/collapse icon color in the other PR, I noticed the splash color on both the icon button and the full expansion panel (if ExpansionPanel.canTapOnHeader is set to true) is just `Theme.of(context).splashColor` on the `InkWell` and `Theme.of(context).highlightColor` on the `IconButton` which may not suit the color scheme of the `ExpansionPanel`, so I have added a custom field `splashColor`, which will effect both the `IconButton` and the full panel `Inkwell`.

…(#147097) (flutter/flutter#147098)

@TahaTesser

@TahaTesser Fix undesirable side effects of your refactor away from my solution to add `IgnorePointer` during your code review of my PR #147098. I don't have time to implement my whole initial fix a second time, and update the tests that were added to verify your disabled color change. The issue is resolved with only adding the IgnorePointer. See [155207](#155207).

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Apr 20, 2024

benji-farquhar closed this Apr 21, 2024

benji-farquhar reopened this Apr 21, 2024

benji-farquhar mentioned this pull request Apr 21, 2024

Add support for ExpansionPanel custom splash color #147126

Merged

9 tasks

TahaTesser self-requested a review April 25, 2024 14:38

This comment was marked as resolved.

Sign in to view

TahaTesser requested changes May 9, 2024

View reviewed changes

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

TahaTesser approved these changes May 9, 2024

View reviewed changes

TahaTesser requested a review from bleroux May 9, 2024 07:35

bleroux approved these changes May 9, 2024

View reviewed changes

TahaTesser added the autosubmit Merge PR when tree becomes green via auto submit App label May 9, 2024

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label May 9, 2024

benji-farquhar and others added 4 commits May 9, 2024 17:33

Fix ExpandIcon color when ExpansionPanel.canTapOnHeader true (#147097)

caca217

Remove trailing space

2e639fd

Set ExpansionPanel ExpandIcon disabled color if canTapOnHeader is true

77b34bc

Update packages/flutter/test/material/expansion_panel_test.dart

477f60a

TahaTesser added the autosubmit Merge PR when tree becomes green via auto submit App label May 9, 2024

auto-submit bot merged commit a5e0c2f into flutter:master May 9, 2024

engine-flutter-autoroll mentioned this pull request May 9, 2024

Roll Flutter from 00f40667c16a to a5e0c2ffe75b (7 revisions) flutter/packages#6703

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 9, 2024

a5e0c2f Fix ExpandIcon color when ExpansionPanel.canTapOnHeader true …

6a46660

…(#147097) (flutter/flutter#147098)

engine-flutter-autoroll mentioned this pull request May 9, 2024

Roll Flutter from 00f40667c16a to bd443998655b (8 revisions) flutter/packages#6704

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 9, 2024

a5e0c2f Fix ExpandIcon color when ExpansionPanel.canTapOnHeader true …

21513d1

…(#147097) (flutter/flutter#147098)

engine-flutter-autoroll mentioned this pull request May 9, 2024

Roll Flutter from 00f40667c16a to 2bfb1b0e2f61 (9 revisions) flutter/packages#6706

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 9, 2024

a5e0c2f Fix ExpandIcon color when ExpansionPanel.canTapOnHeader true …

e99b5ab

…(#147097) (flutter/flutter#147098)

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 6, 2024

a5e0c2f Fix ExpandIcon color when ExpansionPanel.canTapOnHeader true …

72ce7e0

…(#147097) (flutter/flutter#147098)

This was referenced Sep 14, 2024

ExpansionPanel mouse cursor changes when hovering the expand/collapse icon when ExpansionPanel.canTapOnHeader is true #155207

Closed

Fix cursor on hover expand/collapse icon (#155207) #155209

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix ExpandIcon color when ExpansionPanel.canTapOnHeader true (#147097) #147098

Fix ExpandIcon color when ExpansionPanel.canTapOnHeader true (#147097) #147098

Uh oh!

benji-farquhar commented Apr 20, 2024 •

edited

Loading

Uh oh!

flutter-dashboard bot commented Apr 20, 2024

Uh oh!

google-cla bot commented Apr 20, 2024

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

benji-farquhar commented May 8, 2024

Uh oh!

TahaTesser commented May 8, 2024 •

edited

Loading

Uh oh!

benji-farquhar commented May 9, 2024

Uh oh!

Uh oh!

TahaTesser left a comment

Uh oh!

bleroux left a comment

Uh oh!

auto-submit bot commented May 9, 2024

Uh oh!

Uh oh!

Fix ExpandIcon color when ExpansionPanel.canTapOnHeader true (#147097) #147098

Fix ExpandIcon color when ExpansionPanel.canTapOnHeader true (#147097) #147098

Uh oh!

Conversation

benji-farquhar commented Apr 20, 2024 • 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 Apr 20, 2024

Uh oh!

google-cla bot commented Apr 20, 2024

Uh oh!

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

benji-farquhar commented May 8, 2024

Uh oh!

TahaTesser commented May 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benji-farquhar commented May 9, 2024

Uh oh!

Uh oh!

TahaTesser left a comment

Choose a reason for hiding this comment

Uh oh!

bleroux left a comment

Choose a reason for hiding this comment

Uh oh!

auto-submit bot commented May 9, 2024

Uh oh!

Uh oh!

benji-farquhar commented Apr 20, 2024 •

edited

Loading

TahaTesser commented May 8, 2024 •

edited

Loading