Skip to content

3rd party embed player #2217

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 33 commits into from
Dec 21, 2023
Merged

3rd party embed player #2217

merged 33 commits into from
Dec 21, 2023

Conversation

haileyok
Copy link
Member

@haileyok haileyok commented Dec 15, 2023

Continuing from this: #2107

On all devices:
https://streamable.com/p48euh?src=player-page-share

Screenshots

Screenshot 2023-12-19 at 7 02 36 PM
Screenshot 2023-12-19 at 7 02 26 PM
Screenshot 2023-12-19 at 7 02 16 PM
Screenshot 2023-12-19 at 7 02 04 PM
Screenshot 2023-12-19 at 7 01 44 PM
Screenshot 2023-12-19 at 7 01 37 PM
Screenshot 2023-12-19 at 7 01 26 PM

The whole "player" area is pressable to begin playback. The area below the player is pressable to redirect to the external site.

Checklist from the old PR

Twitch parent param, needed origin

From what I can tell, localhost works for the parent param and there is no need for an origin.

Dark mode

No needed changes since this uses the same styling as other external embeds

No Image Render

See below

Android

Setting collapsable to false on the player allows us to measure it.

Remove hack

I'm assuming this was only for being able to use the local dev server. Is that the case?

Testing

Added test cases for parsing links from all embed sources.

Additions and changes from previous PR

Move render logic to ExternalLinkEmbed

  • Memoize the player params
  • Conditionally render the regular external thumbnail or the player view based on whether we have player params or not
  • Allows us to easily keep all the other styling that exists for external embeds

Blur event listener

The webview is not unloaded after we navigate away from whatever screen we are on. Listening for the blur event and running onLeaveViewport() resolves this.

Add SoundCloud

Use the SoundCloud embed URL for tracks and sets (playlists or albums)

Add YouTube Shorts

Uses the same embed as regular YouTube videos. Just added an additional check in the parse function.

Change Spotify album size

Was a tad bit tall on smaller screens and made scrolling away from it difficult. We can go smaller, but then Spotify's styling shrinks the player to the same size as a track. Wasn't a great look to me personally.

Use absolute "layers" for the placeholder and the player

  • Image is rendered whenever a thumbnail is available and the player is either not active or is loading. No re-rendering this image when changing between states
  • Placeholder is placed on top of the image at a zIndex of 2. Changes states from either a play button or an activity indicator.
  • YoutubePlayer or WebView is rendered ontop of the image at a zIndex of 3. This allows us to avoid any flickering or resizing issues. By setting the WebView's background color to transparent, we continue to see the placeholder until the content has loaded.
  • Also avoids any odd differences in rendering between platforms.

No Image Render

This one is a bit annoying, maybe someone has some ideas. Ideally I would like to be able to display something like this, except with the size of the player much shorter, maybe around 80-100px then change the size once the player has been started. (This would be nice for some of the taller players like Spotify/SoundCloud playlists too)

Example Screenshot

Screenshot 2023-12-19 at 6 10 18 PM

The issue with that is when we scroll away, we then set the player to inactive again, which changes the size back and causes a jump in scroll position. I can add some additional logic to check if we have already played the video once and maintain whatever the content's size is.

Alternatively - since I assume that most cases of not having an image are from the poster not completing the thumbnail upload for whatever reason - we could try and fetch it whenever the element comes into the feed, but I'm not sure if that's a good idea or if it's something you'd want to do. So for now, it's just an empty space as shown.

Use StyleSheet API

Just cleaned up a bunch of the inline styles

@pfrazee pfrazee self-assigned this Dec 19, 2023
@haileyok haileyok marked this pull request as ready for review December 20, 2023 03:21
@pfrazee
Copy link
Collaborator

pfrazee commented Dec 21, 2023

This wonderful work again, @haileyok. I ran some QA on all platforms and everything seemed to work just right.

I'm assuming this was only for being able to use the local dev server. Is that the case?

Yep, good to remove.

The issue with that is when we scroll away, we then set the player to inactive again, which changes the size back and causes a jump in scroll position. I can add some additional logic to check if we have already played the video once and maintain whatever the content's size is.

I think this may be the right move for now.

Did I miss any other open questions?

@haileyok
Copy link
Member Author

@pfrazee I think that covers it.

Went ahead and fixed that test so that should pass now. Also moved the useNavigation() hook up to the top of the component (I prefer to keep those up there and it looks like you guys do that in this codebase as well).

@pfrazee
Copy link
Collaborator

pfrazee commented Dec 21, 2023

Great!

@pfrazee
Copy link
Collaborator

pfrazee commented Dec 21, 2023

People are going to really appreciate this. I hope we can get it out soon

@haileyok
Copy link
Member Author

No problem! Real quick, I re-read your comment about the player without an image. Do you want to leave it as is, or do you want it to start at around a height of 80 and resize when we start playing?

@pfrazee
Copy link
Collaborator

pfrazee commented Dec 21, 2023

@haileyok oh right. I'm thinking the move is to make sure the player's updated height gets kept even after it's closed to ensure we don't move the feed around on people

@haileyok
Copy link
Member Author

haileyok commented Dec 21, 2023

Alright. Right now there's no changes at all. It's fine except in the probably rare case where someone would share a spotify or soundcloud playlist (which is the tallest) and no thumbnail would be included. Not really an issue, but might look weird.

So either just keep it like that or set the initial size to 80, then once it's loaded maintain that height even after we remove the webview from scrolling. If you want me to do the latter I'll throw that in later. Otherwise this is good to go.

(Sorry I wan't totally clear about the fact it doesn't jump around right now as it is, just what happens if we change the size on playback then reset it after removing the webview)

@pfrazee
Copy link
Collaborator

pfrazee commented Dec 21, 2023

Okay I'll see how it feels with some testing

Copy link
Collaborator

@pfrazee pfrazee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LFG

@pfrazee pfrazee merged commit fedb94d into bluesky-social:main Dec 21, 2023
@higgins
Copy link

higgins commented Jan 13, 2024

it's circle back season...so excited to see this merged! thank you!!! 🙇‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants