Make chip.dart use WidgetStatesController
#161487
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
framework
ValentinVignal
requested review from
TahaTesser,
bleroux,
nate-thegrate and
victorsanni
ValentinVignal
commented
Jan 12, 2025
Contributor
Author
ValentinVignal
left a comment
ValentinVignal
left a comment
There was a problem hiding this comment.
I had to make the behavior of chips consistent with InkWell to not hit the issue:
The widget on which setState() or markNeedsBuild() was called was:
RawChip
The widget which was currently being built when the offending call was made was:
InkWell
Comment on lines
1030
to
1032
| statesController.update(WidgetState.disabled, !canTap); | ||
| statesController.update(WidgetState.selected, widget.selected); |
Contributor
Author
There was a problem hiding this comment.
I need to use canTap instead of widget.isEnabled as InkWell does its logic on the provided callbacks
Comment on lines
1218
to
1220
| if (_canRawChipTap(oldWidget) != _canRawChipTap(widget)) { | ||
| setState(() { | ||
| setMaterialState(MaterialState.disabled, !widget.isEnabled); | ||
| statesController.update(WidgetState.disabled, !canTap); |
Comment on lines
4732
to
4737
| child: RawChip( | ||
| label: const Text('text'), | ||
| backgroundColor: backgroundColor, | ||
| shape: shape, | ||
| onPressed: () {}, | ||
| ), |
Contributor
Author
There was a problem hiding this comment.
This also introduces this breaking change. A callback needs to be provided in order for the chip to be truly enabled
Contributor
Author
|
@bleroux @nate-thegrate @victorsanni , here is the follow up of what we discussed #159422. I put comments where there are some potential issues that could make this PR not mergeable. Tell me what you think about it |
Contributor
|
Your are right, that it is probably to much of a breaking change. If I understand well, the problem is that Another way would to not share the |
Contributor
|
As far as
I agree with the points @bleroux made. I'm leaning toward not sharing the |
ValentinVignal
force-pushed
the
flutter/make-chips-use-material-states-controller
branch
from
174a7a3 to
7208482
Compare
Contributor
Author
|
Thank you for the feedbacks @bleroux and @nate-thegrate . I changed the code to not share the states controller with Also, I believe this PR can be exempted from tests? |
ValentinVignal
force-pushed
the
flutter/make-chips-use-material-states-controller
branch
from
2de0a76 to
85704ba
Compare
Contributor
nate-thegrate
left a comment
nate-thegrate
left a comment
There was a problem hiding this comment.
LGTM, thanks for following up here!
Contributor
Yeah, I think you're right, since the goal here is to factor out |
nate-thegrate
added
the
refactor
nate-thegrate
approved these changes
Jan 27, 2025
ValentinVignal
force-pushed
the
flutter/make-chips-use-material-states-controller
branch
from
558d1a7 to
b92cb74
Compare
Contributor
Author
|
Any feedback on this @Renzo-Olivares @QuncCccccc ? |
Contributor
|
Sorry for the late response! I will take a look at this PR as soon as possible! |
ValentinVignal
force-pushed
the
flutter/make-chips-use-material-states-controller
branch
2 times, most recently
from
1f59d27 to
f385523
Compare
ValentinVignal
force-pushed
the
flutter/make-chips-use-material-states-controller
branch
from
f385523 to
d6b1ffd
Compare
QuncCccccc
reviewed
Mar 14, 2025
Contributor
QuncCccccc
left a comment
QuncCccccc
left a comment
There was a problem hiding this comment.
Thanks for your patience. Could you help fix the failed tests?
Just a question, do we want to use WidgetStatesController to replace MaterialStateMixin? I'm actually not super familiar with the context, maybe @MitchellGoodwin have any thoughts?
Contributor
Yes, we prefer WidgetStatesController now. MaterialStateMixin has been semi-deprecated for a while. |
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 27, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 27, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 27, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 27, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 27, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 28, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 28, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 28, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 28, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 29, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 29, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 30, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 30, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 31, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 31, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 31, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 31, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 31, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 31, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 31, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Apr 1, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Apr 1, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Apr 1, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Apr 1, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
May 20, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
May 20, 2025
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
May 21, 2025
zhangyuang
pushed a commit
to zhangyuang/flutter-fork
that referenced
this pull request
Jun 9, 2025
Fixes flutter#128289 Follow up of flutter#128507 and flutter#159422 - Makes `RawChip` use `WidgetStatesController` instead of `MaterialStateMixin` - Remove references of `MaterialState` to replace them with `WidgetState` ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --------- Co-authored-by: Qun Cheng <[email protected]>
romanejaquez
pushed a commit
to romanejaquez/flutter
that referenced
this pull request
Aug 14, 2025
Fixes flutter#128289 Follow up of flutter#128507 and flutter#159422 - Makes `RawChip` use `WidgetStatesController` instead of `MaterialStateMixin` - Remove references of `MaterialState` to replace them with `WidgetState` ## Pre-launch Checklist - [x] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [x] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [x] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [x] I signed the [CLA]. - [x] I listed at least one issue that this PR fixes in the description above. - [x] I updated/added relevant documentation (doc comments with `///`). - [x] I added new tests to check the change I am making, or this PR is [test-exempt]. - [x] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [x] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md --------- Co-authored-by: Qun Cheng <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #128289
Follow up of #128507 and #159422
RawChipuseWidgetStatesControllerinstead ofMaterialStateMixinMaterialStateto replace them withWidgetStatePre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.