search

storybookjs / storybook

Storybook is the industry standard workshop for building, documenting, and testing UI components in isolation
https://storybook.js.org
MIT License
84.04k stars 9.24k forks source link

MDX links to stories should use Storybook API instead of doing a full refresh. #9328

Closed patricklafrance closed 4 years ago

patricklafrance commented 4 years ago

Is your feature request related to a problem? Please describe. The current version of MDX linkings support links to other stories though full refresh.

This is quite slow and doesn't leverage existing Storybook API.

Describe the solution you'd like MDX linkings should emit a SELECT_STORY event to navigate to another story.

patricklafrance commented 4 years ago

Here's a few pointers to implement this:

The following code should be updated:

https://github.com/storybookjs/storybook/blob/next/addons/docs/src/blocks/mdx.tsx#L98

It currently append the Manager iframe hostname to the every local URL.

This component is rendered through custom components feature..

Docs available here: https://mdxjs.com/advanced/components

The code providing the custom components is here: https://github.com/storybookjs/storybook/blob/next/addons/docs/src/blocks/DocsContainer.tsx#L20

To test the feature, you can use the official-storybook. The story Addons/Docs/markdown-docs contains a Links section with link to test a local anchor and an anchor to another story.

atanasster commented 4 years ago

@patricklafrance internal anchors might not work, for example the #bottom in the example :

[link to another story](/?path=/docs/addons-docs-docs-only--page#bottom)

Also its probably best to just keep the kind since the anchor is already located in the docs tab.

Here is what I tried let me know what you think:

   if (href.startsWith('#')) {
      return <AnchorInPage hash={href}>{children}</AnchorInPage>;
    }
  // new code starts here
    // allow href="?path=/" and href="/?path=/"
    const storyPath = href.split('?path=/');
    if (storyPath.length === 2) {
      const { storyStore, channel } = useContext(DocsContext);
      const [kind, ...storyIdParts] = storyPath[1].split('/');
      const storyId = (storyIdParts && storyIdParts.length ? storyIdParts.join('/') : kind).split('#')[0];
      const story = storyStore.fromId(storyId);
      if (story) {
        return (
          <A
            onClick={(event: SyntheticEvent) => {
              event.preventDefault();
              channel.emit(SELECT_STORY, story);
            }}
            {...props}
          >
            {children}
          </A>
        );
      }
    }
  ...
patricklafrance commented 4 years ago

That's a good point @atanasster I haven't though about the fact that local anchors might not work after switching to SB API.

From a usage standpoint, it seems very important to me. As a documentation writer, when I link to another page, my intent is almost always to link to a specific section of that page.

@shilman a few days ago on discord you were mentioning new API that we could add to navigate to an anchor, could it also apply to this?

@atanasster could you develop on this?

Also its probably best to just keep the kind since the anchor is already located in the docs tab.

From a user standpoint IMO it would be nice if we could simplify the link to provide.

Instead of:

/?path=/docs/addons-docs-docs-only--page#bottom

I would rather define:

/addons-docs-docs-only--page#bottom
atanasster commented 4 years ago

‘ /addons-docs-docs-only--page#bottom’

Yes, that works

atanasster commented 4 years ago

I checked in the changes, so we can get a conversation going.

Currently looks for href starting with ?path=/ but we could also consider 

[link to another story](story=addons-docs-docs-only--page)
patricklafrance commented 4 years ago

That's a very good idea.

Using story= would also fix: https://github.com/storybookjs/storybook/issues/9302

I don't see any reason to support ?path=/. I'm I missing something?

atanasster commented 4 years ago

I don't see any reason to support ?path=/. I'm I missing something?

Potentially just as a fallback if the storyId is not found in the storyStore. Not sure why :)

@shilman feedback?

patricklafrance commented 4 years ago

I am not sure the storyId will exist in the storyStore if it's a docs only page.

I add to take that into account in https://github.com/storybookjs/storybook/pull/9331

patricklafrance commented 4 years ago

It might be fine though since it's not the same scope? I am not very familiar with the DocsContext and the storyStore.

atanasster commented 4 years ago

I am not sure the storyId will exist in the storyStore if it's a docs only page.

Tried those two and both work (there is a story in the storyStore for the docs only afaics) :

[link to another story](/?path=/addons-docs-csf-with-mdx-docs--basic)
[link to another story](/?path=/addons-docs-docs-only--page)
atanasster commented 4 years ago

Lets wait for feedback, and I guess we can change it to story=xxxx ?

patricklafrance commented 4 years ago

Sounds good!

patricklafrance commented 4 years ago

Still need to figure out how to support local anchors though

shilman commented 4 years ago

Great progress guys. Some thoughts:

Re: story=. What's the rationale for inventing a new linking syntax when markdown already supports one? Just because /?path=/ is ugly?

Re: anchors. The current anchor code triggers a CORS error in "standalone mode" because the preview iframe tries to refer to the parent iframe's URL, which is only valid if the two URLs are on the same host/port. So I proposed adding a new event that the preview can use to trigger an anchor change in the manager to overcome that problem. This event might also be used to solve this issue.

patricklafrance commented 4 years ago

Thanks for your feedback @shilman

For story= well for me it's more about the fact that story= is more intuitive and easier to read. I do prefer that to forcing the documentation writer to remember and hard code the querystring parameter /?path=/.

patricklafrance commented 4 years ago

Another proposition:

Instead of /?path=/addons-docs-docs-only--page it could be /addons-docs-docs-only--page.

That way it's still follow URL syntax.

The code could:

daKmoR commented 4 years ago

perfect is the enemy of good

so imho this

[link to another story](/?path=/addons-docs-docs-only--page)

is good enough for now...

Rationals against:

patricklafrance commented 4 years ago

This feature hasn't been released yet so I don't consider this to be a breaking change.

IMO, introducing /?path=/ now and then changing to something else latter would be a breaking change.

Anyway, it could be /?path=/. Seem weird though that we require the user to specify a queryString parameter that isn't even needed to select the story (I mean the SB event that is emitted to change the story doesn't even need this param, all it need is a story id)

atanasster commented 4 years ago

I would support more the syntax:

/addons-docs-docs-only--page

since I agree with patrick's side that the ?path= seems an unnecessary workaround that could be replaced at some point anyway.

final say @shilman ?

shilman commented 4 years ago

I'm with @daKmoR on this one. If we include the /?path= then it's valid markdown/MDX that can be cut & pasted. No need to reinvent something that already works.

I'd be open to reconsidering this if there were more good reasons to add it, but really not a fan of inventing new syntax when there's already a widely supported standard.

atanasster commented 4 years ago

@shilman - sounds good, the PR is already with ?path=

atanasster commented 4 years ago

@shilman

Re: anchors. The current anchor code triggers a CORS error in "standalone mode" because the preview iframe tries to refer to the parent iframe's URL, which is only valid if the two URLs are on the same host/port. So I proposed adding a new event that the preview can use to trigger an anchor change in the manager to overcome that problem. This event might also be used to solve this issue.

The SELECT_STORY api is already in the manager, does the above note apply to the current issue for using the API?

shilman commented 4 years ago

Jeepers creepers!! I just released https://github.com/storybookjs/storybook/releases/tag/v5.3.0-rc.13 containing PR #9381 that references this issue. Upgrade today to try it out!

You can find this prerelease on the @next NPM tag.

Closing this issue. Please re-open if you think there's still more to do.

lpoulter commented 3 years ago

I don't think this should have been closed. When linking from a story to another a full refresh is still performed in 6.0.1. My mistake a full refresh only happens when putting the link in a comment i'll open an issue.

vinceclicks commented 2 years ago

Can anyone tell me if this feature is supposed to close the SB viewer in Angular?

If I use this link to go from the stories doc page to an MDX file in the same folder, It opens a new page with the MDX shown but without the storybook viewer. [List Item MDX](/story/basin-components-list-item-list-item-mdx--page)

The page it goes to is http://localhost:6006/iframe.html?id=basin-components-list-item-list-item-mdx--page&viewMode=story

There is no list of stories to the side or controls below, no window into viewing the page, just the MDX page itself. I would like to be able to link the two without completely removing the viewer from Storybook

Pickra commented 2 years ago

linking to docs/story page, from another story, is broken for me. I hesitate to make a new issue for this, please let me know if a new issue is necessary.

using

based on this commit, which is this page, i should be able to make a link from 1 story to a docs page/story like this - [button](?path=/docs/components-buttons-iconbuttons--default)

but that gets me this error

Screen Shot 2022-04-11 at 5 54 39 PM

and sure, i can link to the /story/(iframe), but i want to link to a docs only page. what am i missing?

mel-miller commented 2 years ago

and sure, i can link to the /story/(iframe), but i want to link to a docs only page. what am i missing?

+1 for this.

@Pickra — Did you find a solution or workaround for this?

Pickra commented 2 years ago

@mel-miller i think you exclude ?path=. so it should be [link text](/docs/path-to-the-thing)

mel-miller commented 2 years ago

Thanks @Pickra. But, I can't get that to work. I'm going to keep digging.

andapop commented 2 years ago

Thanks @Pickra. But, I can't get that to work. I'm going to keep digging.

+1

franktopel commented 2 years ago

Any news on this?

I just tried to link to a documentation page

[text component](/docs/components-text--default-story#properties)

and it causes Storybook to load this URL:

http://localhost:3000/iframe.html?path=/docs/components-text--default-story#properties

which results in a completely blank page and a console error:

Uncaught Error: Invalid path '/docs/components-text--default-story',  must start with '/story/'
image
alicegherbison commented 4 months ago

Sorry if this has been noted somewhere, but I have the same issue as @franktopel above and can't find any documentation on how to cross-link between .mdx doc page files using Markdown syntax and not have the Storybook screen do a full refresh. Is there a different format of link string or an addon that should be used?