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

UI: Wait 100ms before showing spinner and fix story overlaying it #17753

Merged
merged 7 commits into from
Mar 28, 2022

Conversation

tmeasday
Copy link
Member

Issue: #17122

What I did

  1. Hide user content during preparing, before rendering
  2. Ensure we still show the story during rendering (6.4.1: something removes autofocus from all inputs #16847.
  3. Wait 100ms before showing preparing (Background is flashing when switching stories #17122)

How to test

For 1. see http://localhost:9011/?path=/story/core-loaders--z-index -- try switching to the other loading story, notice the spinner overlay/hides the z-index story

For 2. see http://localhost:9011/?path=/story/core-rendering--auto-focus

For 3. switch between first two backgrounds stories: http://localhost:9011/?path=/story/addons-backgrounds--story-1

@tmeasday tmeasday requested a review from yannbf March 18, 2022 06:38
@nx-cloud
Copy link

nx-cloud bot commented Mar 18, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 0266f52. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@github-actions
Copy link
Contributor

Fails
🚫

PR is not labeled with one of: ["cleanup","BREAKING CHANGE","feature request","bug","documentation","maintenance","dependencies","other"]

Generated by 🚫 dangerJS against 2af2ad4

@shilman shilman changed the title Wait 100ms before showing preparing spinner and avoid story overlaying it. Wait 100ms before showing preparing spinner and avoid story overlaying it Mar 18, 2022
@yannbf
Copy link
Member

yannbf commented Mar 24, 2022

@tmeasday this is great!!! Incredibly better.

I noticed a regression related to loaders, maybe it's out of scope but I thought of sharing:

loadersbug.mp4

@tmeasday
Copy link
Member Author

tmeasday commented Mar 25, 2022

Just to write down what's in the video:

  1. Changing globals re-runs loaders without re-displaying the spinner
  2. Docs mode does not display spinner
  3. Switching back from docs mode->story mode shows a flash of the "old" story.

Playing with next branch, I think only 3 is a regression here.

Happy to discuss 1&2 but I think we can merge this without fixing them.

@tmeasday
Copy link
Member Author

tmeasday commented Mar 25, 2022

@yannbf fixed #3.

Thinking about things for a bit:

  • Fixing 1. would require calling preview.view.showPreparingStory from inside the StoryRender object, which is a bit messy. OTOH I'm not 100% convinced that showing a spinner is the behaviour we want here. Let's discuss.

  • Fixing 2. would require a bit of a refactor of the way the <Story/> doc block works. Definitely a longer term thing. Something to think about in the work we are doing @shilman.

Copy link
Member

@yannbf yannbf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @tmeasday! I agree on your points 👍

Can you check CI? Seems like there's a typescript issue
image

@shilman shilman changed the title Wait 100ms before showing preparing spinner and avoid story overlaying it UI: Wait 100ms before showing spinner and avoid story overlaying it Mar 28, 2022
@shilman shilman changed the title UI: Wait 100ms before showing spinner and avoid story overlaying it UI: Wait 100ms before showing spinner and fix story overlaying it Mar 28, 2022
@shilman shilman merged commit 68a73b8 into next Mar 28, 2022
@shilman shilman deleted the tom/sb-26-wait-100ms-before-showing-preparing branch March 28, 2022 09:28
@shilman shilman added the bug label Mar 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants