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

Show spinner while snapshot image is loading #309

Merged
merged 7 commits into from
May 28, 2024

Conversation

ghengeveld
Copy link
Member

@ghengeveld ghengeveld commented May 16, 2024

  • 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.
Screen.Recording.2024-05-28.at.10.59.09.mov
📦 Published PR as canary version: 1.5.0--canary.309.7b89f8d.0

✨ Test out this PR locally via:

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

@ghengeveld ghengeveld linked an issue May 16, 2024 that may be closed by this pull request
@MichaelArestad
Copy link
Contributor

Nice! I think the spinner is great.

  1. 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.
  2. Can the spinner be vertically centered?

@ghengeveld
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

ghengeveld commented May 17, 2024

@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).

Screen.Recording.2024-05-22.at.14.14.15.mov

@ghengeveld ghengeveld added skip-release Auto: Preserve the current version when merged minor Auto: Increment the minor version when merged labels May 17, 2024
src/components/SnapshotImage.tsx Outdated Show resolved Hide resolved
src/components/SnapshotImage.tsx Outdated Show resolved Hide resolved
@ghengeveld ghengeveld added release Auto: Create a `latest` release when merged and removed skip-release Auto: Preserve the current version when merged labels May 22, 2024
@ghengeveld ghengeveld merged commit d95836b into main May 28, 2024
8 checks passed
@ghengeveld ghengeveld deleted the 256-loader-for-snapshot-image branch May 28, 2024 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Auto: Increment the minor version when merged release Auto: Create a `latest` release when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Loading animation for snapshot image
3 participants