AP-1075 Fix spinner remounting restarts #137
Conversation
There was a problem hiding this comment.
Good work @jgzuke ! The CI failure for check-label is two pronged; first, it's broken because I disabled it but didn't disable it everywhere (my mistake! Fixing in #138). Second, you need to add a label so we can automatically categorize the PR. I added minor for you.
Second, this breaks our chromatic stories because it introduces timing issues. Chromatic, by default, disables css animations so that our visual tests will be consistent run to run, this will change them based on how long the tests take to run. I left a comment explaining how to fix this. Fix it, let me review really quick, and let's get this merged.
Also, please elaborate on why you couldn't test this in AGM. If the readme instructions don't work then let's update them so they do work! Also, and we should avoid this is if we can, we can use npx auto canary to publish a canary build that we can npm install in engine for testing. We should be able to use npm link. One thing that might be causing issues is that the ./run command in AGM automatically runs npm install, which will remove your npm link. There's an environmental variable you can set, SKIP_NPM_INSTALL that will skip the npm install and keep your npm link intact. Give it a whirl!
src/Loaders/LoadingSpinner.tsx
Outdated
| @@ -73,6 +73,9 @@ export const LoadingSpinner: React.FC<Props> = ({ | |||
|
|
|||
| const pixelSize = SIZE_MAP[size]; | |||
|
|
|||
| const mountTime = React.useRef(Date.now()); | |||
| const mountDelay = -(mountTime.current % 540); | |||
There was a problem hiding this comment.
We need to disable this for chromatic testing. Fortunately, Chromatic prescribes a way to do this (https://docs.chromaticqa.com/ignoring-elements):
import isChromatic from 'storybook-chromatic/isChromatic';
You can check the source and see that it checks the user agent, which is a trivial amount of code to add so I'm ok with it.
const mountDelay = isChromatic() ? 0 : -(mountTime.current % 540);
|
Rebase or merge against |
|
Yep I was linking with the npm install skipped, but getting hooks errors from react version mismatches since I believe the dependencies dont get synced the same when linking. I think just pinning the versions should fix it though. Will make the chromatic change |
jgzuke
force-pushed
the
jgzuke/fix-spinner-remounting-jank
branch
from
14d07f1
to
31d04cc
Compare
|
Got the link to work, it looks good in engine 👍 |
|
Great job @jgzuke , LGTM! |
jgzuke
force-pushed
the
jgzuke/fix-spinner-remounting-jank
branch
from
31d04cc
to
8431303
Compare
|
🚀 PR was released in v2.16.0 🚀 |
Take the approach outlined in https://dev.to/selbekk/how-to-stop-your-spinner-from-jumping-in-react-5cmp to stop the spinner from restarting the animation when remounted. This should fix the loader animation restarts described in https://apollographql.atlassian.net/browse/AP-1075.
This worked writing a local story that remounted the component but I was having a really rough time trying to get the linked version to run properly so 🙏 it works properly in that situation as well.