-
Notifications
You must be signed in to change notification settings - Fork 17
Skip startup animations with startDelay #200
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
base: main
Are you sure you want to change the base?
Skip startup animations with startDelay #200
Conversation
✅ Deploy Preview for webkit-jetstream-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
My preference would be to disable them when Those animations only take 1-2 seconds to finish which is pretty negligible with respect to the whole process. I also assume you want to wait to start after loading so you get more consistent results. |
We occasionally run each story individually to gather detailed profiles, adding a couple of seconds does sum up quite a bit. While kinda nice, I don't see any actionable benefit from the animations and IMO the runner harness and page should be as lightweight as possible thus I'm in favour of avoiding unneeded work there. |
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.
Overall LGTM, with comment. No strong opinion either way.
…-14_simpler_startup
I've noticed that generally the startDelay approach doesn't work for us since we have certain hooks that need to run for our infra before the benchmark is ready. And using time delays is notoriously buggy. Further updates:
All in all this will keep the nice jetstreams animations for slow-loading setups. |
const bodyClassList = document.body.classList; | ||
if (JetStreamParams.startDelay !== undefined) { | ||
bodyClassList.remove("animate"); | ||
} |
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.
Oh, I was thinking you'd do this logic before any animation starts in a new script tag between <script src="params.js"></script>
and <script src="JetStreamDriver.js"></script>
You'll have to use document.documentElement.classList
but I think it should work. At least it seems to locally, modulo other CSS bugs I think happen because I removed the other classList changes.
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.
I've first tried to conditionally start the animation – but then I figured that this wouldn't really solve anything when running with our automation. We pause after setting up the page to inject things, thus we can't really use startDelay anyway since it's not fully clear how long the injection-phase lasts.
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.
Would something like an animations=true/false
parameter work? My main concern is that the animations are clipped right now if the tests load in < 1s or so, which looks a little janky.
There are several elements on the page that have a "nice" startup animation.
While visually nice, it means we have to delay measurements in automation to not run into issues with transitions and svg animations occupying resources in the background.