Youtube #960
Youtube #960
Conversation
|
|
||
| abstract class YoutubeWeb { | ||
| factory YoutubeWeb() => getInstance(); | ||
| void createWebInstance(); |
There was a problem hiding this comment.
it doesn't make sense to call it createWebInstance when the native has to implement it as well.
Similarly it does not make sense to call this abstract class YoutubeWeb when you know native class has to implement it as well.
Just call it YouTubeBase
There was a problem hiding this comment.
@fullstackctp think about using export instead of import and have a library, look at webview as an example. It seems cleaner, what do you think?
lib/widget/youtube/youtubestate.dart
Outdated
| @@ -0,0 +1,7 @@ | |||
| import 'package:ensemble/widget/youtube/native/youtube_platform_native.dart' | |||
There was a problem hiding this comment.
this doesn't seem right as you are always importing the native libraries. So the native will be imported even for web? is it import or export?
just look at lib/widget/webview/webviewstate.dart to see how it should be done
There was a problem hiding this comment.
To fix the issue where the youtubeplayer doesn't work sometimes on the ensemble web studio. This issue is actually an issue from the package side.
sarbagyastha/youtube_player_flutter#819
In the issue, it was said to load the WebYoutubePlayerIframePlatform(). This requires importing libraries that work on the web only.
The native library doesn't get imported for the web. It is actually done in the same way as the lottie widget.
export 'native/lottiestate.dart' if (dart.library.html) 'web/lottiestate.dart';
I have tested multiple youtube widgets in the same screen. It doesn't load the video in the player in web. That is why I added the videoList for loading the multiple video in a single player. This problem exists in the demo that they provided. https://sarbagyastha.com.np/youtube_player_flutter/#/
|
|
||
| YoutubeWeb getInstance() => YoutubeNativeInstance(); | ||
|
|
||
| class YoutubeNativeInstance implements YoutubeWeb { |
There was a problem hiding this comment.
doesn't make sense for YoutubeNativeInstance to implement YoutubeWeb. Your code should be understandable and nicely written. This doesn't make sense
|
I understand the issue and saw Vu’s comments on discord. My point is that
you are doing an import and not export. The Lottie code you posted is using
export as well.
All other comments still apply.
…On Fri, Nov 10, 2023 at 3:35 AM fullstackctp ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In lib/widget/youtube/youtubestate.dart
<#960 (comment)>:
> @@ -0,0 +1,7 @@
+import 'package:ensemble/widget/youtube/native/youtube_platform_native.dart'
To fix the issue where the youtubeplayer doesn't work sometimes on the
ensemble web studio. This issue is actually an issue from the package side.
sarbagyastha/youtube_player_flutter#819
<sarbagyastha/youtube_player_flutter#819>
In the issue, it was said to load the WebYoutubePlayerIframePlatform().
This requires importing libraries that work on the web only.
The native library doesn't get imported for the web. It is actually done
in the same way as the lottie widget.
export 'native/lottiestate.dart' if (dart.library.html) 'web/lottiestate.dart';
I have tested multiple youtube widgets in the same screen. It doesn't load
the video in the player in web. That is why I added the videoList for
loading the multiple video in a single player. This problem exists in the
demo that they provided.
https://sarbagyastha.com.np/youtube_player_flutter/#/
—
Reply to this email directly, view it on GitHub
<#960 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACMZUAR46JMWPY7T5OSG67DYDYGQDAVCNFSM6AAAAAA7BO655OVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOMRUGU4TONBXG4>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
@fullstackctp does it fix the multiple youtube players on the page issue? |
|
@fullstackctp please create a ticket or multiple tickets and link here. All your commits must have the ticket as the commit comment |
Yes. The recent commit i just did fixes that
I have added new tickets and linked to this PR. |
kmahmood74
requested review from
snehmehta
and removed request for
vusters and
kmahmood74
|
@snehmehta kindly review this as well. this is deeper into flutter than I am comfortable reviewing |
|
@snehmehta please review this |
Added video position indicator #983 #982