refactor: Add MaterialStateMixin #82843
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 Hixie on the #hackers channel in Chat. 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
bot
added
f: material design
There was a problem hiding this comment.
Do we need to invoke the callback even if this is true?
There was a problem hiding this comment.
This is a good question. My intuition was/is that we do not, but I obviously could be wrong. If an item is currently pressed or hovered, etc, re-entering that same state should both be uninteresting and, even worse, a bug.
If there are any known edge cases that imply re-entering a state is both intended and correct, then I think the answer to your question would change to a strong Yes.
There was a problem hiding this comment.
If re-entering that same state is a bug, then we should assert here if that happens. I am not sure if it is, though. A button could be in the pressed state already (because a finger is touching it) and now a second finger comes down sending it again into the pressed state it is already in. I don't think this is a bug.
Maybe we can just document on the method that the callback only executes if a change in MaterialState occurs?
There was a problem hiding this comment.
Maybe we should rename it from callback to onChanged to make it clear when this fires.
craiglabenz
force-pushed
the
material-state-mixin
branch
from
5e8ad0e to
1e26ec3
Compare
There was a problem hiding this comment.
If re-entering that same state is a bug, then we should assert here if that happens. I am not sure if it is, though. A button could be in the pressed state already (because a finger is touching it) and now a second finger comes down sending it again into the pressed state it is already in. I don't think this is a bug.
Maybe we can just document on the method that the callback only executes if a change in MaterialState occurs?
There was a problem hiding this comment.
Maybe we should rename it from callback to onChanged to make it clear when this fires.
craiglabenz
force-pushed
the
material-state-mixin
branch
from
c8aea93 to
4fd2ec2
Compare
Piinks
left a comment
Piinks
left a comment
There was a problem hiding this comment.
This is awesome! 🎉
Are there other places in the framework where it can be applied? If there are, you don't have to include them in this change, but it may be worthwhile to open a tracking issue for migrating the other classes that could benefit from this. :)
Piinks
added
a: quality
5 participants
Refactors the task of tracking MaterialState values for given widgets into a dedicated
MaterialStateMixinclass. This is added toState<MyWidget>classes to enable simple monitoring of MaterialStates.MaterialStateMixin was first imagined in the Flutter API Design meeting and is documented here.
Pre-launch Checklist
///).