Base ExpansibleStateMixin for ExpansionTile #162702
Conversation
| /// To use this mixin, implement the [buildHeader] and [buildBody] methods to | ||
| /// provide the header and body widgets. The [buildExpansible] method can be | ||
| /// overridden to further customize the layout of the widget. | ||
| mixin ExpansibleStateMixin<S extends StatefulWidget> on TickerProviderStateMixin<S> { |
There was a problem hiding this comment.
I would suggest instead of defining mixin in widget layer, consider create a Base class and expose hook in constructor for material and cupertino to customize this base class.
There was a problem hiding this comment.
Is there a similar example to draw from? As a POC
There was a problem hiding this comment.
editableText, RawMenuAnchor, or any class that start with Raw* or *Base
There was a problem hiding this comment.
one thing to keep in mind as I saw some example here as well, if a property in this class needs to be used by the material class, they should probably be owned from the material and pass into this class, the animationController is one example.
Also, base class should not attempt to do fallback, anything that needs a fall back should be a required pass in parameter (otherwise, you may face an issue where you attempt to do the fallback in multiple layer of the widgets)and the fallback logic should be in the material or cupertino class
There was a problem hiding this comment.
editableText, RawMenuAnchor, or any class that start with Raw* or *Base
RawAutocomplete does something similar with fieldViewBuilder and optionsViewBuilder, I see. So maybe we could have a headerBuilder and a bodyBuilder or something like that.
| /// The controller's [expand] and [collapse] methods cause the | ||
| /// the expansible widget to rebuild, so they may not be called from | ||
| /// a build method. | ||
| class ExpansibleController<S extends StatefulWidget> { |
There was a problem hiding this comment.
Using a base class can also eliminate the S
| Curve get expansionCurve; | ||
|
|
||
| /// True if the widget is initially expanded, and false otherwise. | ||
| bool get initiallyExpanded; |
There was a problem hiding this comment.
If this meant to be control by a controller, do we need this field? if someone care about the initial state, they should probably provide their own controller, otherwise we can default to collapsed
There was a problem hiding this comment.
I agree it's trivial to call controller.expand in initState and that would handle this behavior anyways. But I think the controller is just an add-on, not all cases might want to use it. This is just for convenience in the case where the user cares about the initial state but doesn't use a controller or want to worry about controller logic.
There was a problem hiding this comment.
oh I meant we set the initial state by this field, but uses controller to toggle since a bit weird to me. We should either move the state to controller similar to what TextEditingController does, or move the toggle method to the ExpansionTileState and remove the controller. For the latter, customer will then use globalkey.currentState.expand to control the state
but I see this is existing behavior before the refactor, so probably can be done in different pr
Take 1
Design doc: flutter.dev/go/codeshare-expansion-tile
Fixes Codeshare between ExpansionTile and its Cupertino variant