-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
perf(web): optimize images and modules #7088
Conversation
Deploying with Cloudflare Pages
|
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.
LGTM
9839401
to
a2df314
Compare
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.
lgtm
{:else} | ||
<enhanced:img | ||
loading={preload ? 'eager' : 'lazy'} | ||
src={noThumbnailUrl} |
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.
You should be able to do this and then remove the import
above:
src={noThumbnailUrl} | |
src="@assets/no-thumbnail.png" |
Same elsewhere
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.
Although maybe it's be better to leave the import and then do something like <enhanced:img src={imageData || noThumbnailUrl} />
where you have only a single image. I'm not 100% sure that would work, but I think it should. I might be able to put together a fix for it if it doesn't work
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.
It looks pixelated unless I provide the dimensions with ?w=271;186
, do you know a better way ?
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.
Hmm. Including the dimensions should have no effect since those are the original image dimensions. Do you need to do that when referring to it directly via the src
attribute, when importing it, or both?
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.
Providing the dimensions via the src
or sizes
attributes fix it :
sizes="min(271px,186px)"
or
src="$lib/assets/no-thumbnail.png?w=271;186"
(271x186 are the dimensions of the original picture)
62ea4e7
to
5ec0c6e
Compare
looks like there's a handful of merge conflicts here now |
I'll update it this evening when I get home (unless someone want to do it before) |
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.
lgtm. I'll take a look later on and see if I can simplify things further, but I'm about to leave on vacation and am having trouble getting the server to run locally, so I'd say let's merge this and then if I figure out any way to simplify things when I get back I'll send a followup
Thanks ben, enjoy your vacation ! 😄 |
…e-images-and-modules
Optimize build size and use
@sveltejs/enhanced-img
Screenshots