Conversation
There was a problem hiding this comment.
I think Flutter has a convention of pinning dependencies, i.e. video_player: 0.0.2
There was a problem hiding this comment.
This is a very long build method, I suggest refactoring it a bit. For example, the onTap builds a whole screen, could that be a separate widget (i.e. FullScreenVideo)?
There was a problem hiding this comment.
Good idea. I split it into a couple of helper methods. Hope it reads nicer.
There was a problem hiding this comment.
Can't controller by null here, too?
There was a problem hiding this comment.
Not in version 0.0.3!
There was a problem hiding this comment.
You can use the => short form here.
There was a problem hiding this comment.
You can use the => short form here.
There was a problem hiding this comment.
We should avoid abbreviations. imageFadeAnimation
There was a problem hiding this comment.
We can avoid non-Flutterish formatting by inserting some trailing commas in the following lines.
There was a problem hiding this comment.
Can't we use an AnimatedWidget here? The code seems pretty low-level.
There was a problem hiding this comment.
It would be nice.
Looked into AnimatedOpacity, but it transitions from the current value into the new. I want opacity to go from 1.0 to 0.0 each time. Not sure how it would work.
Also looked into ImplicitlyAnimatedWidget, it has the needed logic, but adds a lot of extra complications.
Leaving this for now
There was a problem hiding this comment.
Avoid ugly indentation here with a trailing comma.
sigurdm
left a comment
sigurdm
left a comment
There was a problem hiding this comment.
PTAL
Notice I changed the LoopingController to keep the future for being able to dispose correctly
There was a problem hiding this comment.
It would be nice.
Looked into AnimatedOpacity, but it transitions from the current value into the new. I want opacity to go from 1.0 to 0.0 each time. Not sure how it would work.
Also looked into ImplicitlyAnimatedWidget, it has the needed logic, but adds a lot of extra complications.
Leaving this for now
|
thanks for this PR! :) |
There was a problem hiding this comment.
This phrase can be interpreted as antagonistic to an American audience. Is there another option we could use?
There was a problem hiding this comment.
How about:
new VideoCard(
title: 'Butterfly',
subtitle: 'flutters by',
),
new VideoCard(
title: 'Bee',
subtitle: 'gently buzzing',
),
?
There was a problem hiding this comment.
sure :)
bonus points if you can write a haiku!
sigurdm
force-pushed
the
gallery_video_demo
branch
from
d14cfaf to
21b206a
Compare
| final String subtitle; | ||
|
|
||
| const VideoCard({Key key, this.controller, this.title, this.subtitle}) | ||
| : super(key: key); |
There was a problem hiding this comment.
Could be
const VideoCard({
Key key,
this.controller,
this.title,
this.subtitle,
}) : super(key: key);though I don't know if dartfmt leaves it that way.
There was a problem hiding this comment.
Dartfmt ruins this. Leaving as is.
| final TransitionRoute<Null> route = new PageRouteBuilder<Null>( | ||
| settings: new RouteSettings(name: title, isInitialRoute: false), | ||
| pageBuilder: (BuildContext context, Animation<double> animation, | ||
| Animation<double> secondaryAnimation) { |
There was a problem hiding this comment.
You could avoid this deep nesting by extracting the onTap handler and/or the pageBuilder as locals above.
| controller.pause(); | ||
| } else { | ||
| imageFadeAnimation = new FadeAnimation( | ||
| child: new Icon(Icons.play_arrow, size: 100.0)); |
There was a problem hiding this comment.
imageFadeAnimation = new FadeAnimation(
child: new Icon(Icons.play_arrow, size: 100.0),
);| final Duration duration; | ||
|
|
||
| const FadeAnimation( | ||
| {this.child, this.duration: const Duration(milliseconds: 500)}); |
There was a problem hiding this comment.
const FadeAnimation({
this.child,
this.duration: const Duration(milliseconds: 500),
});| content: const ListTile( | ||
| title: const Text('No network'), | ||
| subtitle: const Text( | ||
| 'To load the videos you must have an active network connection'), |
| super.initState(); | ||
| connectivitySubscription = | ||
| connectivityStream().listen((ConnectivityResult connectivityResult) { | ||
| print(connectivityResult); |
| void initState() { | ||
| super.initState(); | ||
| connectivitySubscription = | ||
| connectivityStream().listen((ConnectivityResult connectivityResult) { |
There was a problem hiding this comment.
Dunno if this might help:
connectivitySubscription = connectivityStream().listen(
(ConnectivityResult connectivityResult) {
...
},
);| class _VideoDemoState extends State<VideoDemo> | ||
| with SingleTickerProviderStateMixin { | ||
| final VideoPlayerController butterflyController = | ||
| new VideoPlayerController(butterflyUri); |
There was a problem hiding this comment.
Trailing comma. Though this one is a little annoying.
final VideoPlayerController butterflyController = new VideoPlayerController(
butterflyUri,
);| void initState() { | ||
| super.initState(); | ||
| animationController = | ||
| new AnimationController(duration: widget.duration, vsync: this); |
| final GlobalKey<ScaffoldState> scaffoldKey; | ||
|
|
||
| const ConnectivityOverlay( | ||
| {this.child, this.connectedCompleter, this.scaffoldKey}); |
| } | ||
| } | ||
|
|
||
| // Shows a |
|
Looks like this PR did not include automatic changes to the gallery's android/ and ios/ directories, which is interfering with people testing with the gallery. |
|
Fix for Android in #13361 |
6 participants
No description provided.