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: lazy load images #1969

Merged
merged 5 commits into from
Apr 26, 2023
Merged

feat: lazy load images #1969

merged 5 commits into from
Apr 26, 2023

Conversation

Shinigami92
Copy link
Member

@Shinigami92 Shinigami92 commented Apr 11, 2023

@Shinigami92 Shinigami92 self-assigned this Apr 11, 2023
@stackblitz
Copy link

stackblitz bot commented Apr 11, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@netlify
Copy link

netlify bot commented Apr 11, 2023

Deploy Preview for elk-docs canceled.

Name Link
🔨 Latest commit b434838
🔍 Latest deploy log https://app.netlify.com/sites/elk-docs/deploys/64494c39f1adfe00085f38c8

@netlify
Copy link

netlify bot commented Apr 11, 2023

Deploy Preview for elk-zone ready!

Name Link
🔨 Latest commit b434838
🔍 Latest deploy log https://app.netlify.com/sites/elk-zone/deploys/64494c39b255830008e0cade
😎 Deploy Preview https://deploy-preview-1969--elk-zone.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 settings.

@Shinigami92
Copy link
Member Author

Works also on my iOS iPhone XS in Opera

@Shinigami92 Shinigami92 marked this pull request as ready for review April 11, 2023 13:14
antfu
antfu previously approved these changes Apr 11, 2023
@antfu
Copy link
Member

antfu commented Apr 11, 2023

Why remove the img.onload hook? It's supposed to render the blurhash before the image is fully downloaded.

@Shinigami92
Copy link
Member Author

Why remove the img.onload hook? It's supposed to render the blurhash before the image is fully downloaded.

Because it would load every image twice! (I tested it)
Because of the lazy it now always falls into the static 3 second timeout. So the programmatic onload got never called anymore.

@Shinigami92 Shinigami92 requested a review from antfu April 12, 2023 03:28
@antfu
Copy link
Member

antfu commented Apr 12, 2023

But then, with this change, you don't have the blur brush anymore. I guess to have the best of both world, we could have the blurbrush image behind the image so it still renders before the image it loaded. Then we could hook @load="" to the real image and remove the blurbrush once the image if fully loaded

@Shinigami92 Shinigami92 marked this pull request as draft April 14, 2023 20:46
@Shinigami92
Copy link
Member Author

@Shinigami92 Shinigami92 marked this pull request as ready for review April 25, 2023 17:39
Copy link
Member

@antfu antfu left a comment

Choose a reason for hiding this comment

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

Looking good 👍

@johannschopplich
Copy link

johannschopplich commented Apr 25, 2023

Hey @Shinigami92 – you were fast! I was about to open a PR with unlazy. Thank you for the effort.

Some notes:
I implemented the lazyLoad prop for the Nuxt UnLazyImage component today specifically for Elk, to make the shouldLoadImage prop compatible. The latter is used for the data saving mode. With the lazyLoad prop we can control if the BlurHash should be generated, but no image yet loaded:

<UnLazyImage :lazy-load="shouldLoadImage" />

If shouldLoadImage is false, then only the BlurHash will be generated, but the image not loaded. This makes the CommonBlurHash component now compatible with data saving mode in Elk again. There was a bug in unlazy v0.8.2 but fixed in v0.8.4. Could you please update and test again? Thanks!


Regarding ssr: false in the module config – I'm not sure if this is necessary. I believe that none of the components are server-side rendered anyway, since data is fetched in the client before the newsfeed is populated. But I'm not 100 % certain how Elk works under the hood.

@Shinigami92
Copy link
Member Author

scrolled through the preview app and seems to work 👌

@ayoayco
Copy link
Member

ayoayco commented Apr 26, 2023

@Shinigami92 check this weird behavior on the deployed preview. The page is reloaded when switching tabs.
lazy-loading-elk

@userquin
Copy link
Member

@ayoayco can you check if images have width and height set?

@ayoayco
Copy link
Member

ayoayco commented Apr 26, 2023

@ayoayco can you check if images have width and height set?

@userquin I do see width and height properties on <img> elements that are set

Screenshot 2023-04-26 at 1 55 39 PM

@Shinigami92
Copy link
Member Author

If anybody wants to try to fix this tab behavior, feel free to take over the PR, because I don't know what the Unlazy component is doing behind the scene

@johannschopplich
Copy link

johannschopplich commented Apr 26, 2023

Honestly, I'm unsure why this reload happens. Maybe because of some service worker magic under the hood in the preview environment? UnLazyImage should only load images and shouldn't trigger a whole newsfeed refresh. I will take a deeper look later. I'd really see unlazy in use here so I hope we find the origin of this reload behaviour.
@Shinigami92 I've made some minor perf improvements to unlazy today. Would you mind updating to v0.8.8? Thanks!

@Shinigami92
Copy link
Member Author

Would you mind updating to v0.8.8?

Yeah will do later, need to rest currently 😴

@patak-dev
Copy link
Member

I don't think this issue is related to this PR. I was also able to reproduce it with other PRs like #2019. So maybe it is a regression in our test user. (also interesting, in your video you aren't always signed in to the Elk Dev Team Test user 🤔)

@ayoayco
Copy link
Member

ayoayco commented Apr 26, 2023

I think @patak-dev also saw this on other PRs' preview environments! Will also check later.

@ayoayco
Copy link
Member

ayoayco commented Apr 26, 2023

(also interesting, in your video you aren't always signed in to the Elk Dev Team Test user 🤔)

Yes it was signed out to make sure both environments were fetching from the same server (ie, because elk dev test user fetches from universeodon), to compare behavior but was hard due to auto reload.

@socket-security
Copy link

New dependency changes detected. Learn more about Socket for GitHub ↗︎


👍 No new dependency issues detected in pull request

Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore [email protected] bar@* or ignore all packages with @SocketSecurity ignore-all

Pull request alert summary
Issue Status
Install scripts ✅ 0 issues
Native code ✅ 0 issues
Bin script shell injection ✅ 0 issues
Unresolved require ✅ 0 issues
Invalid package.json ✅ 0 issues
HTTP dependency ✅ 0 issues
Git dependency ✅ 0 issues
Potential typo squat ✅ 0 issues
Known Malware ✅ 0 issues
Telemetry ✅ 0 issues
Protestware/Troll package ✅ 0 issues

📊 Modified Dependency Overview:

➕ Added Package Capability Access +/- Transitive Count Publisher
@unlazy/[email protected] None +4 johannschopplich

@Shinigami92
Copy link
Member Author

I cant reproduce @ayoayco behavior on my local machine

Peek.2023-04-26.18-22.mp4

Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

It works great for me! Awesome work @johannschopplich and @Shinigami92!

@patak-dev patak-dev merged commit 23c1dfe into main Apr 26, 2023
@patak-dev patak-dev deleted the 1954-lazyload-images branch April 26, 2023 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Very slow text entry on Safari in long image threads
6 participants