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

Core: Show "booting" progress until story is specified or errors #20425

Merged
merged 34 commits into from
Jan 18, 2023

Conversation

tmeasday
Copy link
Member

@tmeasday tmeasday commented Dec 28, 2022

Issue: #20250

What I did

Show the progress bar on the preview component until we learn the story is specified or an error occurs.

If preview.js takes a long time to load, the story specified event will not fire until it is done.

Screenshot 2022-12-28 at 9 02 47 pm

A (potential) downside here is you always see the above briefly while the SB is loading.

How to test

Add a "speed loop" to a preview.js, then load the SB:

const start = new Date();
while (new Date() - start < 2000);

Copy link
Contributor

@JReinhold JReinhold left a comment

Choose a reason for hiding this comment

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

I think this is a good first step, makes it way better at least.

But in general I'm not a fan of showing a progress bar to users when progress can't be determined, but that's a minor thing. We probably need an indetermined loading spinner or similar in general in our UI.

code/ui/manager/src/components/preview/preview.tsx Outdated Show resolved Hide resolved
code/ui/manager/src/components/preview/preview.tsx Outdated Show resolved Hide resolved
@yannbf
Copy link
Member

yannbf commented Dec 28, 2022

A (potential) downside here is you always see the above briefly while the SB is loading.

What if you added a timeout to only show this after like 100-200ms?

@tmeasday
Copy link
Member Author

tmeasday commented Dec 29, 2022

We probably need an indetermined loading spinner or similar in general in our UI.

One issue we are trying to avoid is having too many different spinners[1] flash up in the early stages, so I think a better UI would be some version of a progressbar that is clearly indeterminate.

[1] First we show the loading progress bar as the build is compiled, then we show this thing while preview.js is executed, then we show a docs skeleton or spinner while the docs/story is imported.

@tmeasday
Copy link
Member Author

Actually @JReinhold @yannbf I changed my mind on this. Looking closer, we already show a circular spinner for a moment when a SB first starts (before the entry loads) so I might as well keep it simple and simply augment the existing logic to wait a bit longer (for the story to be specified by the preview -- which happens after initialization -- rather than when configured).

@yannbf
Copy link
Member

yannbf commented Dec 29, 2022

Sounds good and looks better! Could you check the behavior with a composed storybook?

This is all we had for refs too, and the logic for keeping it simple is the same.
@tmeasday
Copy link
Member Author

tmeasday commented Jan 3, 2023

Thanks for the prompt to check properly @yannbf. When I did so I ran into some problems and decided I was just adding some extra complexity to some code that was more complex than it needed to be already. So I simplified things to just have a ready state (same as we do in refs) that gets set when the story is selected or an error is emitted. That works in all the cases we are concerned about.

@ndelangen you should probably check this as the original author.

@yannbf
Copy link
Member

yannbf commented Jan 3, 2023

I was testing it out and saw that between loading states, there are "blank" states, please let me know if that is intended:

Here's a "Fast 3G" network setting, to see it a little slower, with cache disabled:
2023-01-03 10 19 17

Here's no throttling, with cache disabled and refreshing a couple times to see the behavior better (it's similar with cache as well):
2023-01-03 10 19 52

@shilman shilman changed the title Core: Show a "booting" progress message until the story is specified Core: Show "booting" progress until story is specified or errors Jan 3, 2023
@ndelangen
Copy link
Member

@tmeasday I'll do a proper review of this next week.

@tmeasday
Copy link
Member Author

tmeasday commented Jan 6, 2023

No, definitely not intended @yannbf, I'll take a closer look.

@tmeasday
Copy link
Member Author

tmeasday commented Jan 8, 2023

Ok, I improved it, although we still need to figure out the old events. Thanks for the tip @yannbf. I think this also highlights that we remove the DOCS_PREPARING UI before we actually have finished rendering the docs underneath. Not really noticeable in normal network conditions, but I wonder if we should fix it. In this PR or another?

@yannbf
Copy link
Member

yannbf commented Jan 12, 2023

@tmeasday maybe stories should be called entries instead? or something along those lines?

@ndelangen
Copy link
Member

ndelangen commented Jan 12, 2023

@tmeasday I'm taking a look at those errors!

The typings of globalThis for this package are defined here:

declare var __STORYBOOK_ADDONS_MANAGER: any;

I was able to get it green, but not without a minor changes to a laundry-list of typings.d.ts files.
I don't know what caused this to appear.
I can't say it makes a ton of sense..

I'll open a telescope PR, so I can show the changes, and we can discuss further.

@tmeasday
Copy link
Member Author

Thanks @ndelangen !

@tmeasday
Copy link
Member Author

@ndelangen @yannbf OK, this is ready to go I think, apart from the globals issue.

As discussed with @yannbf I:

  • Changed the name from stories/storiesHash to index as it's no just stories any more.
  • Added deprecated getters for the old properties, as we have a documented useStorybookState API (for better or worse)

…eck-work-valid

make changes to typings files to ensure this the check command succeeds
@tmeasday
Copy link
Member Author

Thanks for making it green! Any change of a ✅ from @ndelangen or @yannbf?

@ndelangen
Copy link
Member

@tmeasday will put a calender event om my monday morning to review 👍

@tmeasday tmeasday merged commit 007d11a into next Jan 18, 2023
@tmeasday tmeasday deleted the tom/sb-1123-sb20250-vite-does-not-show-a-spinner-2 branch January 18, 2023 09:01
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.

4 participants