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: fetchpriority #42

Open
leoj3n opened this issue Mar 5, 2024 · 8 comments
Open

feat: fetchpriority #42

leoj3n opened this issue Mar 5, 2024 · 8 comments
Labels
documentation Improvements or additions to documentation

Comments

@leoj3n
Copy link
Contributor

leoj3n commented Mar 5, 2024

I don't see any support for fetchpriority="high" here. I'm thinking this is something you may want to add.

To use priority hints with picture tags simply add the fetchpriority attribute to the img element inside the picture tag.

<picture>
  <source srcset="/image-small.png" media="(max-width: 600px)">
  <img src="/image.png" fetchpriority="high">
</picture>

https://www.debugbear.com/blog/priority-hints#picture-elements-and-priority-hints

Take a Largest Contentful Paint image, which, when preloaded, will still get a low priority. If it is pushed back by other early low-priority resources, using Fetch Priority can help how soon the image gets loaded.

https://web.dev/articles/fetch-priority#summary

@zerodevx
Copy link
Owner

zerodevx commented Mar 6, 2024

Right now unknown component props are forwarded to the internal <img> tag, so:

<Img {src} fetchpriority="high" />

will be rendered similar to your attached code - but not for media attribute though. Most of the time (I think), one could use sizes to pick up the correct image instead.

@leoj3n
Copy link
Contributor Author

leoj3n commented Mar 7, 2024

@zerodevx I thought you might reply along these lines. I would point out that other props you have implemented fall under the same caveat. Not sure what is the best way forward, but as long as you have implemented export let loading = 'lazy' and export let decoding = 'async' (which is far less useful) I'm not sure you can argue not to include this.

Edit: The media attribute is from where I copy-pasta the example. I have not needed to use media= myself yet.

@zerodevx
Copy link
Owner

zerodevx commented Mar 7, 2024

The reason why loading and decoding are added is because they change <img> tag defaults - IMO unless we're changing fetchpriority defaults there isn't a strong case to define it explicitly?

@leoj3n
Copy link
Contributor Author

leoj3n commented Mar 7, 2024

Can you give an example of code output? Not understanding what you mean by changing img tag defaults.

@zerodevx
Copy link
Owner

zerodevx commented Mar 8, 2024

I meant the HTMLElement <img> tag default values; specifically loading and decoding for which we are overriding the default value. Hence:

/** @type {Object} imagetools import */
export let src = {}
/** @type {string|undefined} img tag `sizes` attr */
export let sizes = undefined
/** @type {number|undefined} img width override */
export let width = undefined
/** @type {number|undefined} img height override */
export let height = undefined
/** @type {'lazy'|'eager'} img tag `loading` attr */
export let loading = 'lazy'
/** @type {'async'|'auto'|'sync'} img tag `decoding` attr */
export let decoding = 'async'
/** @type {HTMLImageElement|undefined} bindable reference to `<img>` element */
export let ref = undefined

These explicitly declared props are not arbitrary - each of them has a reason behind why it's declared.

I'm not understanding the reason for explicitly declaring fetchpriority at the moment - unless you are recommending to override the default value.

@leoj3n
Copy link
Contributor Author

leoj3n commented Mar 8, 2024

@zerodevx I see what you are saying. My original thought was to include it for educational purposes for the users of svelte-img. I would have it be unset, so by default the browser can still decide via it's heuristics the best priority.

Off-topic: I guess it's an OK idea to always default to lazy for loading, but I'm not so sure it's needed to set decoding.

Anyways I think it is nice to be explicit and mention all the different toggles that are available that one will probably want to use with this component, at the very least in the docs. Me from a couple weeks ago might have just picked up this library and ran with it without coming across fetchpriority/loading/decoding. You might say it's not your job / the job of this repo to educate but I would argue a lib like this is ground zero for someone looking for a quick solution and it is helpful place to give some advice. But I understand if you think differently and prefer to keep more of a laser focus here.

If you were to make a typedef and add these to that it would show up in the in the IDE which might be nice. I suppose in that case you would be tempted to extend HTMLImgAttributes which kind of obscures these critical options again. So, at least a note in the README would be nice... But I won't be surprised if you are of the opinion that people should find out about these options elsewhere.

Feel free to close; just am curious to hear how you think about this.

@zerodevx
Copy link
Owner

mention all the different toggles ... at the very least in the docs

That could be a lot of props. 😅 But I think we could mention the salient ones in the docs. Will be delighted to take a PR for it.

If you were to make a typedef...

That's a good alternative too - don't have to hint every prop, but salient ones like what you mentioned might be helpful.

prefer to keep more of a laser focus

Generally yes - I'd normally lean towards providing just a base set of opinions, but allowing effects to be overridden if the consumer is more informed or inclined to do so.

@zerodevx zerodevx added the documentation Improvements or additions to documentation label Mar 11, 2024
@leoj3n
Copy link
Contributor Author

leoj3n commented Mar 11, 2024

That could be a lot of props.

I'm pretty sure I was thinking fetchpriority/loading/decoding as "toggles" people should know about what others were you thinking?

I will submit a PR later with some types with comment that would show up in IDE when using the component.

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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants