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

Image optimization process #111

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Image optimization process #111

wants to merge 14 commits into from

Conversation

sas-news
Copy link

@sas-news sas-news commented Apr 4, 2024

Add a downloadPoster prop to YouTube and Vimeo and LinkPreview components.
Download images at build time, using getImage().

Copy link

changeset-bot bot commented Apr 4, 2024

⚠️ No Changeset found

Latest commit: 3cb480c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

netlify bot commented Apr 4, 2024

Deploy Preview for astro-embed ready!

Name Link
🔨 Latest commit 7495ca8
🔍 Latest deploy log https://app.netlify.com/sites/astro-embed/deploys/660dfd8370486d0008de8d01
😎 Deploy Preview https://deploy-preview-111--astro-embed.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Owner

@delucis delucis left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @sas-news! I also considered if this might be something worth exploring.

Thinking about this from the big picture, I don’t love requiring a prop to control something that in theory could be a sensible default. Especially when the prop doesn’t work without extra configuration — it’s almost always a nicer API if you only have one place to check to make sure it’s configured correctly.

I think what might make sense is:

  1. Always use getImage() under the hood. If the remote URL is not authorized, inferSize still works, even if no optimization happens. (One case to double check would be using a non-absolute URL, e.g. if someone has an image in public/, e.g. poster="/example.jpg" should continue to work.)
  2. Document the optimization available by adding to image.domains in astro.config.mjs
  3. (optional) Maybe services with predictable image URLs can export an array people can spread into their config if they want

I think that would match the likely use case that people want the download behaviour consistently for their whole site, not on a component-by-component basis.

What do you think?


P.S. There might also be some follow-up opportunities here. For example, often the OG images are much larger than needed (1200px wide), so if we’re using getImage() with an authorized domain, there’s also the opportunity to resize it to a more appropriate size.

@sas-news sas-news reopened this Apr 17, 2024
@sas-news
Copy link
Author

Correction of image optimization process

  • For LinkPreview component, use Image component for image optimization
  • For YouTube and Vimeo components, use getImage() for image optimization
  • Do not perform optimization for unauthorized remote URLs
  • Document that optimization can be enabled by configuring image.domains in astro.config.mjs
  • Deprecate downloadPoster prop as it is no longer needed

This allows local images to work correctly out of the box, while still providing the ability to opt-in to optimization for approved remote domains. LinkPreview component now benefits from the built-in optimization capabilities using astro:assets, while YouTube and Vimeo components use getImage() for optimization.

@sas-news sas-news requested a review from delucis April 18, 2024 22:08
@sas-news sas-news changed the title Add downloadPoster prop Correction of image optimization process Apr 19, 2024
@sas-news sas-news changed the title Correction of image optimization process Image optimization process Apr 20, 2024
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.

2 participants