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: Add preloading to stories highlighted in the sidebar #17964

Merged
merged 2 commits into from
Jul 28, 2022

Conversation

ndelangen
Copy link
Member

What I did

Optimizations:

  • When you hover over a story in the sidebar we could sent a message to the preview the user is likely to go to that story.
  • When a user navigates to a story using the keyboard shortcuts we could sent a message to the preview the user is likely to go to that story.

@nx-cloud
Copy link

nx-cloud bot commented Apr 14, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit df77b98. 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.

@ndelangen ndelangen self-assigned this Apr 14, 2022
@shilman shilman changed the title add preloading to stories getting highlighted in the sidebar UI: Add preloading to stories highlighted in the sidebar Apr 26, 2022
Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

I tried this out and found two problem:

  1. When mousing over the sidebar, it only preloads at the story level, which is "too late" as a sibling story will already have been loaded. (similar when using the keyboard).

  2. It doesn't do anything in the search panel. I thought we planned to do similar behaviour on mouse/keyboard selection?

lib/ui/src/components/sidebar/Tree.tsx Outdated Show resolved Hide resolved
Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

Meant to request changes

@ndelangen ndelangen requested a review from tmeasday April 26, 2022 12:32
@ndelangen
Copy link
Member Author

@tmeasday I think I made all the requested changes

Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

Functionally it works great now.

I did notice the target attribute doesn't seem consistent:

image

image

Also, I wonder if we should just add an api.preloadStory method to the stories module to avoid repeating the logic all over the place.

lib/preview-web/src/PreviewWeb.tsx Outdated Show resolved Hide resolved
@ndelangen ndelangen requested a review from tmeasday April 28, 2022 09:26
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.

I tested in different projects using webpack4 and 5 and the behavior is super awesome!

There was one scenario that didn't go really well (kind of expected to be honest @tmeasday) which is when using LazyCompilation. If the user hovers over +10 stories fast, then webpack goes haywire:

webpack5lazycompilation.mov

@ndelangen
Copy link
Member Author

Should report as a bug to webpack I guess.

I propose we add a debounce here:
https://github.com/storybookjs/storybook/pull/17964/files#diff-1c750d3374a0e928fde974aae523e9257f6b6c23321e70527841850683609419R212-R226

@tmeasday
Copy link
Member

tmeasday commented May 9, 2022

Ok! Wanna add it in this PR or another next cycle?

@ndelangen
Copy link
Member Author

I'd be in favor of merging this, and creating a new task for next cycle

Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

@ndelangen there seems to be some stories failing

lib/ui/src/components/sidebar/SearchResults.tsx Outdated Show resolved Hide resolved
@ndelangen ndelangen requested a review from tmeasday July 18, 2022 14:53
@ndelangen ndelangen changed the base branch from next to future/base July 18, 2022 17:15
lib/preview-web/src/PreviewWeb.tsx Outdated Show resolved Hide resolved
lib/ui/src/components/preview/preview.tsx Outdated Show resolved Hide resolved
lib/ui/src/components/sidebar/Tree.tsx Outdated Show resolved Hide resolved
@ndelangen ndelangen requested a review from tmeasday July 19, 2022 07:55
Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

LGTM

Base automatically changed from future/base to next July 25, 2022 10:37
auto map the ref id in lib/api when calling emit
emit preload event when hovering over sidebar items or selecting them with the keyboard
@ndelangen ndelangen force-pushed the norbert/sb-28-prefetch-csf-files-on-hover-select-via branch from cbc0e45 to 1aa9f79 Compare July 27, 2022 11:44
@ndelangen ndelangen merged commit 82cf379 into next Jul 28, 2022
@ndelangen ndelangen deleted the norbert/sb-28-prefetch-csf-files-on-hover-select-via branch July 28, 2022 10:28
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