-
-
Notifications
You must be signed in to change notification settings - Fork 175
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
Adds check for either WidgetStates or MaterialStates in useMaterialStatesController debug test #421
Conversation
|
), | ||
equalsIgnoringHashCodes( | ||
'HookBuilder\n' | ||
' │ useMaterialStatesController: WidgetStatesController#00000({})\n' |
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.
This feels a bit weird to me. useMaterialStatesController
is explicitly instantiating MaterialStatesController
How did we end-up with a WidgetStatesController
? Wouldn't we get a type error?
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.
Oh MaterialStatesController
is deprecated and it's a typedef
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.
It is weird. MaterialStatesController
is being moved down to the widgets layer and renamed to WidgetStatesController
. For migration purposes, there is still a MaterialStatesController
, but it is just a typedef of WidgetStatesController
, so it acts the same as how it did before. Except for the debug functions, which return strings mentioning WidgetState
.
Considering MaterialStatesController is deprecated, the failing "hook" should be deprecated too. Does this PR have to land before #421? I'd rather:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #421 +/- ##
=======================================
Coverage 98.36% 98.36%
=======================================
Files 21 21
Lines 858 858
=======================================
Hits 844 844
Misses 14 14 ☔ View full report in Codecov by Sentry. |
I saw that the Flutter PR broke the test in this package, so I wanted to add a fix for the test that would pass both before and after the change in Flutter. Theoretically #421 can land before flutter/142151, then a But if you'd rather not adjust the upcoming broken test and add a |
Awesome. I'd rather keep things are they are right now then. Thanks a lot for the heads up though! |
No problem. I'll close the PR 👍 |
Flutter is moving MaterialState, and it's related functions outside of the Material library and changing it's name to WidgetState. MaterialState will still be available as is, just marked deprecated, and made a typedef of WidgetState. However this causes MaterialState debug functions to return with strings mentioning "WidgetState", and is a breaking change for packages using those functions. The PR that will add that change to Flutter is here, and the original issue is here.
Internal testing found this caused a breakage in this package, so this PR updates the test for
useMaterialStatesController
to pass both before and after that change is landed.