Show spinner while snapshot image is loading

chromaui / addon-visual-tests

Visual Tests addon for Storybook

MIT License

28 stars 1 forks source link

Show spinner while snapshot image is loading #309

Closed ghengeveld closed 1 month ago

ghengeveld commented 1 month ago

Show a spinner while the snapshot image (latest/baseline, whichever is active) is loading.
Prevent showing the diff/focus overlay image while the snapshot is loading.
Show a spinner while the overlay image (diff/focus) is loading, on top of the snapshot image.

https://github.com/chromaui/addon-visual-tests/assets/321738/966a6f18-0bcf-4208-8bdb-f56dd4941ae3

📦 Published PR as canary version: 1.5.0--canary.309.7b89f8d.0

:sparkles: Test out this PR locally via: ```bash npm install @chromatic-com/storybook@1.5.0--canary.309.7b89f8d.0 # or yarn add @chromatic-com/storybook@1.5.0--canary.309.7b89f8d.0 ```

MichaelArestad commented 1 month ago

Nice! I think the spinner is great.

Can you keep the spinner in place until all of the images are loaded? I'm not sure the pulsing image conveys it's not ready yet.
Can the spinner be vertically centered?

ghengeveld commented 1 month ago

Can you keep the spinner in place until all of the images are loaded? I'm not sure the pulsing image conveys it's not ready yet.

I could, but then we're not showing the user something useful while we could be. It's a micro optimization but I prefer that over showing the spinner for longer. In the Chromatic webapp we currently preload the snapshot image but we don't preload the overlay images, which inspired this behavior.

Can the spinner be vertically centered?

It is, but the snapshot in the demo is < 100px tall and I've given the container a min-height: 100px, so it's actually getting vertically centered relative to that. Without the min-height the spinner could render flush to the top edge or even get clipped, which would be weird.

MichaelArestad commented 1 month ago

I could, but then we're not showing the user something useful while we could be. It's a micro optimization but I prefer that over showing the spinner for longer. In the Chromatic webapp we currently preload the snapshot image but we don't preload the overlay images, which inspired this behavior.

@ghengeveld In that case, I would recommend keep current behavior, but without the pulsing image.

It is, but the snapshot in the demo is < 100px tall and I've given the container a min-height: 100px, so it's actually getting vertically centered relative to that. Without the min-height the spinner could render flush to the top edge or even get clipped, which would be weird.

Perfect!

ghengeveld commented 1 month ago

@MichaelArestad Without the pulsating effect, there is no way to know the overlay image is still loading, which may lead people to incorrectly assume there are no visual changes.

An alternative effect I originally envisioned is a linear indeterminate progress bar either above or below the image. Do you think that would be more appropriate?

I implemented this alternative loading indicator. It is actually simpler because it works both for the snapshot image and the overlay images (in which case the bar appears on top of the snapshot image).

https://github.com/chromaui/addon-visual-tests/assets/321738/beddb417-3074-44f2-9e65-e59f4f84e55e