Implemented various improvements #2
Conversation
ahmednfwela
commented
Jul 31, 2020
ahmednfwela
commented
- Added a builder instead of child
- Separated preview widget from the full screen widget
|
I like that the builders are separated 👍 Would directly solve my current issue. |
| final Color backgroundColor; | ||
| final bool backgroundIsTransparent; | ||
| final DisposeLevel disposeLevel; | ||
|
|
||
| const FullScreenWidget({ | ||
| Key key, | ||
| this.builderWithChild, |
There was a problem hiding this comment.
Why not do either/or? So either provide a child or a builder, this is more in line with the usage of builders in the rest of Flutter.
Then assert that only one is provided, before the super
: assert(child = null | builder = null), super(key: key)
There was a problem hiding this comment.
i can split the constructor into 3 factories, one with builder, another with child and another with both child and builder
what do you think about that ?
There was a problem hiding this comment.
Don't think that makes it much better. Point was that it'd be weird to have multiple builders that can be defined at the same time, without any errors, though it will actually choose one.
| @@ -120,7 +135,7 @@ class _FullScreenPageState extends State<FullScreenPage> { | |||
| } | |||
| } | |||
|
|
|||
| _endVerticalDrag(DragEndDetails details) { | |||
| void _endVerticalDrag(DragEndDetails details) { | |||
| if (positionYDelta > disposeLimit || positionYDelta < -disposeLimit) { | |||
| Navigator.of(context).pop(); | |||
There was a problem hiding this comment.
Would be nice if we were to be able to access this context through the builder somehow. Especially for the non-preview version.
There was a problem hiding this comment.
the non-preview version is in a different route, thus a different context.
What exactly do you want to access with the preview context?
