-
Notifications
You must be signed in to change notification settings - Fork 26.8k
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
Add support for ExpansionPanel custom splash color #147126
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 "@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. |
dc43666
to
fb4199a
Compare
fb4199a
to
b61ce4d
Compare
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.
To reconfirm, if canTapOnHeader
is true, is there no way to trigger the icon splash at all?
Also, can you add tests?
Co-authored-by: Tong Mu <dkwingsmt@users.noreply.github.com>
8a2130b
to
7a44582
Compare
@dkwingsmt Correct. The Icon shouldn't splash if I will add tests. |
@dkwingsmt I have implemented the changes, I noticed I might have exposed a bug with You can test with this in
|
Hi @dkwingsmt, can we progress this? It should be visibly working by running the example code in my above comment, and the widget tests verify the colors get passed through to the underlying widgets. |
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.
LGTM
The change looks great! I also ran the demo above, and I see that in the |
Add doc comments to clarify when splashColor will be ignored
5acc96a
to
41d35d4
Compare
Thanks @dkwingsmt. I have updated the test to verify that the underlying There was some confusion around the fact that
However, I have updated the doc comments to inform the developer when |
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.
LGTM! Great finding!
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.
LGTM 👍
Cheers @dkwingsmt & @justinmc. Good to merge? |
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`.
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 theInkWell
andTheme.of(context).highlightColor
on theIconButton
which may not suit the color scheme of theExpansionPanel
, so I have added a custom fieldsplashColor
, which will effect both theIconButton
and the full panelInkwell
.Pre-launch Checklist
///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.