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: Allow overriding WebView and UrlStore #19623

Merged
merged 24 commits into from
Nov 8, 2022

Conversation

tmeasday
Copy link
Member

Issue: React Native wants to override these with non-web versions.

What I did

  1. Allow passing the view+store into PreviewWeb
  2. Create a super class with the properties we need
  3. Type everything

How to test

Everything should just pass as before in SB proper.

@shilman I am labelling this as patch because we want to backport to 6.5.x. However it may not apply cleanly due to the type parameterization. We could do a simpler version that doesn't worry about that?

@tmeasday tmeasday added feature request patch:yes Bugfix & documentation PR that need to be picked to main branch labels Oct 25, 2022
@tmeasday tmeasday requested review from shilman and dannyhw October 25, 2022 23:58
@shilman shilman added maintenance User-facing maintenance tasks and removed feature request labels Oct 26, 2022
@shilman
Copy link
Member

shilman commented Oct 26, 2022

@tmeasday classifying this as a maintenance PR so we can patch it back to 6.5 😈

@dannyhw
Copy link
Member

dannyhw commented Oct 26, 2022

This is great! Thanks for making this happen 🙏

@tmeasday
Copy link
Member Author

Decision: Rename renderToDom to renderToRoot (TStorybookRoot maybe?) and WebProjectAnnotations -> ProjectAnnotations).

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Generally LGTM, but are we going to rename Store_WebProjectAnnotations to ProjectAnnotations in a subsequent PR?

@tmeasday
Copy link
Member Author

@shilman I'm doing it in this PR, will be done momentarily.

@tmeasday
Copy link
Member Author

tmeasday commented Nov 1, 2022

@kasperpeulen after piping the TStorybookRoot through everything that has a TFramework, I'm now thinking I should have just made it part of TFramework -- a third optional parameter that defaults to HTMLElement. WDYT about that?

I guess I would have to do something like:

export type AnyStorybookFramework = AnyFramework & { TStorybookRoot: any };
export type AnyWebStorybookFramework = AnyStorybookFramework & { TStorybookRoot: HTMLElement };

export class PreviewWeb<TFramework extends AnyStorybookFramework> {... }

export interface ReactFramework extends AnyWebStorybookFramework {
  component: ComponentType<this['T']>;
  storyResult: StoryFnReactReturnType;
}

WDYT?

@tmeasday
Copy link
Member Author

tmeasday commented Nov 2, 2022

Take another look @kasperpeulen and @shilman - we can still move the Framework type up to csf if we want:

export interface Framework extends AnyFramework {
rootElement: unknown;
}

@kasperpeulen
Copy link
Contributor

Looks good @tmeasday
I feel this should end up in CSF (both Framework and WebFramework).
As I don't see why this is really Storybook specific.

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Looks great to me! 🙌

@JReinhold JReinhold removed their assignment Nov 2, 2022
code/lib/preview-web/src/Preview.tsx Outdated Show resolved Hide resolved
code/lib/preview-web/src/PreviewWeb.tsx Outdated Show resolved Hide resolved
code/lib/preview-web/src/View.ts Outdated Show resolved Hide resolved
code/lib/preview-web/src/render/Render.ts Show resolved Hide resolved
@tmeasday tmeasday requested a review from ndelangen November 4, 2022 03:09
@tmeasday tmeasday changed the title Allow overriding WebView and UrlStore Core: Allow overriding WebView and UrlStore Nov 4, 2022
@tmeasday tmeasday added the core label Nov 4, 2022
@ndelangen ndelangen merged commit 226f9a4 into next Nov 8, 2022
@ndelangen ndelangen deleted the tom/sb-850-allow-passing-webview-and-urlstore-to branch November 8, 2022 16:18
@tmeasday tmeasday removed the patch:yes Bugfix & documentation PR that need to be picked to main branch label Nov 21, 2022
@tmeasday
Copy link
Member Author

tmeasday commented Nov 21, 2022

@shilman I removed the patch label as I've created #19904 for the backport.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants