-
-
Notifications
You must be signed in to change notification settings - Fork 3
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
Lite Youtube #485
Lite Youtube #485
Conversation
View diff of compiled files (may take a few minutes): https://github.com/oddbird/oddleventy-built/compare/main..lite-youtube |
* main: Update issue templates fix error in issue template winging-it.yml
* main: lint
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.
Looks good!
The default lite-youtube poster images are rather low quality, so this uses the 11ty open graph API to get a higher quality image. We have a few options on the poster frames-
- host our own version of the API
- rely on 11ty's instance of the API (as in this PR)
- Provide a way for users to specify a poster image that we host in the site
- Have crappy images
Hm, what's your leaning here? It seems like options 1 or 2 would be preferred -- is there a downside to using the 11ty hosted version? I guess hosting our own is probably more robust, and doesn't look like it'd be difficult.
I'm also open to feedback on the video macro- matching on the string in the video source doesn't feel right, but it allows us not change existing embeds (although there aren't many), and gives us a consistent interface for all embed.videos. Alternatively, we could add an
embed.youtube(id)
macro.
I think string matching is okay in this case, although adding a type: 'youtube'
arg would be fine too -- we could be more strict about matching the entire string, e.g. https://www.youtube.com/embed/
.
My preference is to rely on 11ty's instance. The downside is that any point it may go down, or be turned off, and 11ty specifically states there is no uptime guarantee. In that scenario, we can simply remove the provided background image, and fall back to the lower quality ones until we come up with an alternate solution. |
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.
Code looks good to me, but I'm not seeing anything in the deploy preview. Confusing.
@mirisuzanne What does "not seeing anything" mean in this case? Do you see the embedded videos on e.g. https://deploy-preview-485--oddleventy.netlify.app/2017/01/31/css-day/? @jamesnw I notice that most of the embedded videos on the site are calling the |
@jgerigmeyer ok, I see it working there. Looks good. But then I'm confused why it's not being used for most of our youtube embeds - like https://deploy-preview-485--oddleventy.netlify.app/2023/11/22/winging-it-04/ |
@jgerigmeyer and @mirisuzanne Before I transition more content over, can you take a look at the changes to the macro that I made, and make sure it matches what you're thinking? |
@jamesnw It looks on the right track to me. After you port the content, we may want to reconsider the usefulness of the separate |
I agree - looks like the right approach, and I think we should probably get rid of the video macro. Just use figure everywhere. |
I applied the On https://deploy-preview-485--oddleventy.netlify.app/talks/css-is-rad/ things are slightly out of alignment- The live site is slightly more aligned- A similar alignment issue is on other tiled layouts with a Vimeo link- https://deploy-preview-485--oddleventy.netlify.app/talks/dynamic-css/ and https://deploy-preview-485--oddleventy.netlify.app/talks/intrinsic-web/ |
@jamesnw I added support, but also discovered that the alignment issue is due to how vimeo embeds handle pixel rounding (or something like that) - so it depends on the exact viewport width. When I resize, the youtube embeds resize smoothly, while vimeo embeds are a bit jumpy. Usually things align, and then at some sizes the vimeo embeds lose a couple pixels above and below. I don't think that's on our end. |
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.
Looks good! @jamesnw @mirisuzanne is this ready to merge?
Description
Replaces iframe Youtube embeds with
lite-youtube
, wrapped in anis-land
component. The default lite-youtube poster images are rather low quality, so this uses the 11ty open graph API to get a higher quality image. We have a few options on the poster frames-I'm also open to feedback on the video macro- matching on the string in the video source doesn't feel right, but it allows us not change existing embeds (although there aren't many), and gives us a consistent interface for all embed.videos. Alternatively, we could add an
embed.youtube(id)
macro.Related Issue(s)
#482
Steps to test/reproduce
Compare 2017/01/31/css-day/ to https://oddbird.ing/2017/01/31/css-day/