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

[feature] Disable GIF animation unless user hovers #2986

Merged

Conversation

hydrosquall
Copy link
Contributor

PR Checklist

Please check all that apply to this PR using "x":

  • I have checked that this PR is not a duplicate of an existing PR (open, closed or merged)
  • I have checked that this PR does not introduce a breaking change
  • This PR introduces breaking changes and I have provided a detailed explanation below

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting)
  • Refactoring (no functional changes)
  • Documentation changes
  • Other - Please describe:

Fixes

Issue Number: #2029

What is the current behavior?

  • GIFs play all the time, making the CPU/GPU work hard on GIF intensive pages

What is the new behavior?

  • GIFs only play if the user's mouse hovers over them. Tested using the lbry://@3DPrinting address.

Other information

Since we don't control the source of these GIFs as they are submitted by users, I felt that the solutions for requesting a separate static image, using CSS keyframes, or manually creating a canvas were not applicable, even though those other solutions would probably get even better performance.

@hydrosquall hydrosquall changed the title [feature] Disable GIF animation on user hover [feature] Disable GIF animation unless user hovers Oct 6, 2019
@neb-b
Copy link

neb-b commented Oct 7, 2019

Thank you for the PR!

I'm not sue if I want to add this dependency for this one feature. bundlephobia says it alone will add 400ms on 3g networks.
https://bundlephobia.com/result?p=freezeframe@4.0.0-alpha.6

Is there any way this could be achieved without adding freezeframe.js?

@hydrosquall
Copy link
Contributor Author

hydrosquall commented Oct 7, 2019

Hi @seanyesmunt ,

Thanks for the suggestion, I had forgotten that this wasn't just a desktop app that gets installed once, and that this code is also being use for the web application.

I took a look at the bundle results inside https://chrisbateman.github.io/webpack-visualizer/, and found that ~36% of the bundle size came from core-js (which is already a project dependency) and 36% came from common-tags, which could be eliminated in favor of using a regular template literal instead of a tagged template literal.

With that motivation in mind, I extracted the areas of code + branches that we were using from freezeframe.js, and inlined them into the component folder. Each file has a URLs attributing the files back to the source. I checked on the desktop electron app, and it works the same as we had it before. See this commit: 7cf7b96

This lets us whittle dependencies down to just the imagesloaded package, which adds 34 ms on emerging 3G.

https://bundlephobia.com/result?p=imagesloaded@4.1.4

Let me know if there are additional changes you'd like to see.

@neb-b
Copy link

neb-b commented Oct 8, 2019

Nice work. That seems a lot more reasonable. I'll give this a closer look later today!

@neb-b
Copy link

neb-b commented Oct 10, 2019

Hey sorry it took so long to review this. Just tested and it works great!

Can you add a change to this file? https://github.com/lbryio/lbry-desktop/blob/master/CHANGELOG.md

then it's ready to merge.

@tzarebczan
Copy link
Contributor

@hydrosquall, thanks for the PR! Can we show you some appreciation for the contribution?

Also, we're giving away some Hacktoberfest bonuses and goodies for this month. We'll get it all figured out after you shoot us an email after this is reviewed/merged.

The only other comment I'd make for this one is to show some kind of indicator on the thumbnail that there's a preview for it. Thoughts @seanyesmunt? Otherwise you may not even know you can mouse over it.

@hydrosquall
Copy link
Contributor Author

hydrosquall commented Oct 11, 2019

Thanks @seanyesmunt , I've updated the changelog in 80d507e

Hi @tzarebczan, thanks for the suggestion - I'll send an email after this is merged.

Regarding an indicator: the original freezeframe.js had an "arrow icon overlay", see the third demo at the page below:

http://ctrl-freaks.github.io/freezeframe.js/

I think it can be incorporated, perhaps in a follow-up PR? Since we no longer depend on freezeframe directly, we can use any icon that we'd like (maybe something from the lbry brand?)

Separately, I've bundled up this smaller version of the library as a new NPM package:

@hydrosquall hydrosquall force-pushed the feature/disable-gif-animation-unless-hover branch from ef6e37f to 4b0afa4 Compare October 11, 2019 00:33
@hydrosquall hydrosquall force-pushed the feature/disable-gif-animation-unless-hover branch from 4b0afa4 to 97a1920 Compare October 11, 2019 00:34
@hydrosquall hydrosquall force-pushed the feature/disable-gif-animation-unless-hover branch from 97a1920 to 80d507e Compare October 11, 2019 14:23
@neb-b neb-b merged commit 41c1392 into lbryio:master Oct 13, 2019
@kauffj
Copy link
Member

kauffj commented Oct 14, 2019

Great contribution @hydrosquall!

@tzarebczan
Copy link
Contributor

And don't forget to follow up about Hacktoberfest / appreciation :)

@hydrosquall hydrosquall deleted the feature/disable-gif-animation-unless-hover branch October 29, 2019 01:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants