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.

Sign up for GitHub

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

Jump to bottom

Do you accept PR that actually fix performance? #32

Open
wardpeet opened this issue Dec 1, 2017 · 4 comments
Open

Do you accept PR that actually fix performance? #32

wardpeet opened this issue Dec 1, 2017 · 4 comments

Comments

@wardpeet
Copy link

wardpeet commented Dec 1, 2017

No description provided.

@scottschiller
Copy link
Owner

scottschiller commented Dec 2, 2017 •
edited
Loading

I will assume this was written with good intentions in mind, but comes off as snarky / frustrated - presumably because you saw that performance was sub-par, and/or, there haven't been any updates on this repo in some time. Notwithstanding, I appreciate the note because we're all human and I feel like this myself on occasion. "Dammit, does anyone still actually maintain this thing!?" 😉

If you have suggestions or ideas in the form of a PR or similar for improving performance, that is most welcome! I typically update this script yearly, before "snowstorm season" hits in November. I did not get to updates before November this year, but might have to patch something in for Firefox (see below). This script has been around since 2003, so it has quite a history to it (and should still work in IE 6, perhaps older.)

An issue was just reported in Firefox Quantum (57), it appears to have a regression in style calc/layout when there are a number of position: fixed elements on the screen. In this case, position: fixed is used where supported to make snow "stick" to the bottom of the window while scrolling. I've suggested a few options for workarounds.

Firefox 57 findings / recommendations here. #31 (comment)

It would be nice to get accelerated snow via CSS animations / transitions (albeit, animations might have to be simplified a bit), and/or transform: translate() for positioning instead of the current absolute top/right/bottom/left. The present approach uses the latter and is very convenient because percentage-based offsets can be used, and they are very flexible in that snow will automatically reposition relative to the screen (or parent element if someone wants it snowing only in one <div>, etc.)

@wardpeet
Copy link
Author

wardpeet commented Dec 3, 2017

Thanks for your message. I didn't want to come of as snarky, so sorry about that. I don't mind putting a PR together that fixes the things you are referring too. Lots of advertisements are using this script so that's why I wanted to update this plugin as I see plenty of people are using it.

I didn't want to write this issue as a criticism on your work as you mentioned this plugin was created in 2003 and the web grew and grew during that time.

I'll cook up a PR :)

@ericnkatz
Copy link

@scottschiller Thanks for writing and revising this over the years. 👍

@scottschiller
Copy link
Owner

scottschiller commented Dec 9, 2017 •
edited
Loading

Damn advertising! 🤛 🔥 🤜
Well, if it inspires more people to install ad blockers due to CPU usage, I call that a net win. :trollface:

I like the idea of <canvas> for a rendering target these days, potentially lower CPU than CSS animations. I just noted the site saddlebackleather.com running some snow (at time of writing, Dec. 2017), which appears to be a static background image (or two) with long CSS transitions. Firefox isn't great on CPU (80%?), but Chrome was surprisingly high - like 180% CPU on this 2011-era quad-core i7. Maybe it's because the snow is on <body> or similar and runs the whole page despite the animation only showing in the header, who knows. Regardless, that one's notably worse than Snowstorm - I didn't expect that, honestly.

<canvas> remains of interest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@ericnkatz @scottschiller @wardpeet