Skip to content
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

feat: ✨ YouTube Playlist Component #36

Merged
merged 10 commits into from
Oct 23, 2022

Conversation

Omafovbe
Copy link
Contributor

@Omafovbe Omafovbe commented Oct 9, 2022

Created a component to accommodate listing the youtube playlist. for now just the images and their titles.

Related Issue

resolves #17

Summary of Changes

@thescientist13 thescientist13 added question Further information is requested feature New feature or request labels Oct 9, 2022
@thescientist13 thescientist13 self-requested a review October 9, 2022 22:02
Copy link
Contributor

@thescientist13 thescientist13 left a comment

Choose a reason for hiding this comment

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

Hey @Omafovbe , thanks for the PR!

Would it be possible to see this in a Storybook? That would be really helpful for review and testing. You can see how the Footer component is structured to get an idea of how to set things up.

Left some feedback in-line and also a couple minor observations (no big deal, I can always clean these up before merge)

  • If you run yarn lint it should help give you some feedback on indentation fixes to help us keep our code formatting a bit more consistent
  • I would avoid abberviations, I think it's fine to spell out the whole file / variable name, e.g. src/components/youtube-playlist/youtube-playlist.js
  • Looks like a package-lock.json file landed in the PR, so we can delete that.

Thanks for the good start, let me know if you have any questions!

@@ -0,0 +1,6 @@
channel ID: UCxs5mxoDpmmR0hRbwsxU7Sg
Copy link
Contributor

@thescientist13 thescientist13 Oct 9, 2022

Choose a reason for hiding this comment

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

API / implementation details like this information may be better captured in the Storybook for this component.

@@ -0,0 +1,25 @@
const fetchVideoPlaylist = async () => {
const API_KEY = '';
let playlists = await fetch(`https://youtube.googleapis.com/youtube/v3/playlists?part=snippet&channelId=UCxs5mxoDpmmR0hRbwsxU7Sg&key=${API_KEY}`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, I hadn't thought of fetching them ahead of time and listing them out. My only thought with this approach is as we are planning on pre-rendering the site to HTML, it would be great to avoid fetches on the client. But this, could also be a neat technique to fetch latest videos on page load too, to append any new items to the existing list. This would avoid having to re-build the page since I'm not sure what kind of web hooks YouTube offers.


Also wonder what does it look like if the playlist is just embedded on the page in an <iframe>?

<iframe width="560" height="315" src="https://www.youtube.com/embed/qRskZcB5Rbc" title="YouTube video player" frameborder="0" allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture" allowfullscreen></iframe>

Curious to see what each option looks like. <iframe> approach may mean a little less control stylistically, but we never have to worry about stale data, it will always be the latest on page load.


Just thinking out loud but would be good to experiment with the <iframe> approach too, to know our options. We can always start out with an <iframe> version so it is ready by Tuesday, and build our own at a later date using your technique here, if we can get access to web hooks or something like that. Will do some research on my end.

To test, we could put a hardcoded example of the <iframe> version into the Storybook template to compare . contrast.

@@ -35,4 +35,10 @@ a {

h6 {
text-align: center;
}

.videos {
Copy link
Contributor

@thescientist13 thescientist13 Oct 9, 2022

Choose a reason for hiding this comment

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

We're using Tailwind in this project, so it would be good to try and stick to using those classes since it is utility based, otherwise I think at a component level, an inline style="..." is good enough for now for one-off styles tweaks.

Here's a little cheat sheet for getting started with Tailwind. (this is my first project using it, so far so good!)

}
}

customElements.define('yt-playlist', Playlists);
Copy link
Contributor

Choose a reason for hiding this comment

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

Our current convention has been to prefix all first party custom elements with tt-, so for this example it would be

customElements.define('tt-youtuve-playlist', YouTubePlaylist);

@Omafovbe
Copy link
Contributor Author

Omafovbe commented Oct 10, 2022

In relation to all of the comments you made on the first PR, I have done some update

  1. Updated the yt-playlist branch from feature/cast-and-crew branch
  2. Carried out all of suggestions (hope I didn't miss any one)
  3. Create storybook files to help preview the updates and changes
  4. I really don't know how to manage the YouTube channel ID on storybook

Here are some image preview of how things look at the moment

youtube playlist2

youtube playlist1

Please let me know if there is anything I missed and need updating. Thanks

@Omafovbe
Copy link
Contributor Author

Omafovbe commented Oct 11, 2022 via email

@thescientist13 thescientist13 added hacktoberfest-accepted documentation Improvements or additions to documentation labels Oct 11, 2022
Copy link
Contributor

@thescientist13 thescientist13 left a comment

Choose a reason for hiding this comment

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

Hey @Omafovbe , thanks for your work here and providing the different samples.

I think we should have an <iframe> embed version to compare with, just so we can see what a "live" list version might look like just coming straight from YouTube. That could be the simplest to start out with for now and then maybe we could evolve the implementation together in follow up PRs? But for now, let's just compare both options (<iframe> vs API call) just so we can learn what the pros / cons of each approach might be.

Also, we would want to specifically use the playlist from the issue, as that is a playlist from our channel specific to our Tuesday's Tunes show. So basically, the embed version should probably look something like

<iframe src="https://www.youtube.com/watch?v=qRskZcB5Rbc&list=PLrY8SmqJ5XZ_UvVurEvGg8i10g-cxMudZ" ...>

Here are the docs from YouTube to help give you an idea of what might be possible with that option.
https://developers.google.com/youtube/player_parameters


As a note on branch management, I would just focus on maintaining a reference to only what is in the v1 branch. Each PR only has to focus on one task of work, so no need to mix and match work from other PRs. If you could just make sure your branch is coming from v1 and doesn't have any other code other than the YT work, that will make sure reviewing (and merging!) is much easier for each of us.

Thanks again for your help! ✌️

@thescientist13 thescientist13 mentioned this pull request Oct 12, 2022
5 tasks
Copy link
Contributor

@thescientist13 thescientist13 left a comment

Choose a reason for hiding this comment

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

Let me know if you need any help finishing this up. There was a PR opened for the Spotify player which should give you a good reference for what to do for this PR. So basically just cleaning up the extra files and just using the <iframe> for now. - #43

I did make a ticket to track a more long term solution for building our players so if you have any thoughts / ideas on that approach, feel free to comment! - #45

@Omafovbe
Copy link
Contributor Author

Omafovbe commented Oct 16, 2022 via email

@thescientist13 thescientist13 removed the question Further information is requested label Oct 16, 2022
Copy link
Contributor

@thescientist13 thescientist13 left a comment

Choose a reason for hiding this comment

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

Thanks for your help with this @Omafovbe ! 🙌

Going to keep it as an <iframe> for now and will think about other ways to enhance this as you had originally suggested, I like the idea of us creating a first party playlist for this component.

Feel free to jump into any other tasks you might find, I'll be making a few more issues over the next couple days and so some of those should be good first issue types as well.

@thescientist13 thescientist13 merged commit 2d56d28 into AnalogStudiosRI:v1 Oct 23, 2022
thescientist13 added a commit that referenced this pull request Oct 25, 2022
* feat: ✨ Youtube Playlist Component

Created a component to accommodate listing the youtube playlist. for now just the images and their titles.

* initial version of cast and crew section

* basic styling and theming of the header

* add test cases

* refactor: 🎨 updated component to display both youtube in iframe and img playlist

Saved the Youtube data API response in two separate json files.

Added stories for quick display

* delete cast and crew component

* update YouTube player implementation to an iframe

* branch cleanup

* add test case

* restore cast and crew files

Co-authored-by: Owen Buckley <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation feature New feature or request hacktoberfest-accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Youtube Playlist
2 participants