-
Notifications
You must be signed in to change notification settings - Fork 7
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
Add support for embedding playlists #296
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: d8dbccb The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Copilot
AI
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 5 out of 8 changed files in this pull request and generated 1 suggestion.
Files not reviewed (3)
- packages/youtube/test/embed.default.test.js: Evaluated as low risk
- packages/youtube/test/_urls.mjs: Evaluated as low risk
- demo/src/youtube.md: Evaluated as low risk
Comments skipped due to low confidence (2)
packages/youtube/lib/embed.js:87
- The use of 'id ?? playlist' should be reviewed to ensure it doesn't introduce unintended behavior when both are present or missing.
return `https://i.ytimg.com/${fileTypePath}/${id ?? playlist}/${fileName}`;
packages/youtube/lib/embed.js:181
- [nitpick] The error message should be reviewed for consistency with other error messages in the codebase.
console.info("Couldn’t fetch video title. Falling back to default...\n", error);
); | ||
}); | ||
test(`Build embed lite mode, 1+ index, responsive true`, t => { | ||
t.is(embed(extract(testString), override({lite: { responive: true }}), 1), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in the property name 'responive' should be 'responsive'.
t.is(embed(extract(testString), override({lite: { responive: true }}), 1), | |
t.is(embed(extract(testString), override({lite: { responsive: true }}), 1), |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
Closes #149
At very long last, this PR adds support for YouTube playlists. There are two use cases:
https://www.youtube.com/playlist?list=PLFtSvldL7Mh4ismj4BgH33pBR9hbtBkxz
https://www.youtube.com/watch?v=C04JZsoqs1A&list=PLFtSvldL7Mh4ismj4BgH33pBR9hbtBkxz&index=4
Known limitations in Lite mode
lite-youtube-embed
doesn't recognize playlist IDs so if there's no video ID, the embed won't display anything in Lite mode. But if it finds a video ID (such as in the second example above) it will embed just that video.