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

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

Closed
patricklafrance opened this issue Jan 5, 2020 · 32 comments

Comments

@patricklafrance
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

@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
Copy link
Member Author

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
Copy link
Member

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

Yes, that works

@atanasster
Copy link
Member

atanasster commented Jan 6, 2020

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
Copy link
Member Author

That's a very good idea.

Using story= would also fix: #9302

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

@atanasster
Copy link
Member

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
Copy link
Member Author

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 #9331

@patricklafrance
Copy link
Member Author

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

@atanasster
Copy link
Member

atanasster commented Jan 6, 2020

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
Copy link
Member

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

@patricklafrance
Copy link
Member Author

Sounds good!

@patricklafrance
Copy link
Member Author

Still need to figure out how to support local anchors though

@shilman
Copy link
Member

shilman commented Jan 7, 2020

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
Copy link
Member Author

patricklafrance commented Jan 7, 2020

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
Copy link
Member Author

patricklafrance commented Jan 7, 2020

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:

  • check if the URL is relative
  • if it is, it parse the storyId out of the URL and check if it match a story in the store.
  • if it does, on click it emit the SELECT_STORY (or whatever new event we come up with)

@daKmoR
Copy link
Contributor

daKmoR commented Jan 7, 2020

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:

  • /addons-docs-docs-only--page => needs a server that supports catch-all request (e.g. even though a subfolder is requested to send the request to an index.html or so) also what happens if it is an actual valid folder?
  • story= => imho people wanna copy past... e.g. go to a page and then copy the url... at least that is what I am doing... so adjusting the url would need to be a system-wide change... probably breaking? can be reconsidered for 6.x?

@patricklafrance
Copy link
Member Author

patricklafrance commented Jan 7, 2020

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
Copy link
Member

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
Copy link
Member

shilman commented Jan 7, 2020

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
Copy link
Member

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

@atanasster
Copy link
Member

@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
Copy link
Member

shilman commented Jan 11, 2020

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
Copy link

lpoulter commented Sep 25, 2020

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
Copy link

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
Copy link
Contributor

Pickra commented Apr 11, 2022

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

  • OS: mac
  • node: v14.17.0
  • typescript
  • webpack 5
  • "@storybook/addon-actions": "6.4.19",
  • "@storybook/addon-essentials": "6.4.19",
  • "@storybook/addon-interactions": "6.4.19",
  • "@storybook/addon-links": "6.4.19",
  • "@storybook/addons": "6.4.19",
  • "@storybook/builder-webpack5": "6.5.0-alpha.48",
  • "@storybook/core-common": "6.4.19",
  • "@storybook/manager-webpack5": "6.5.0-alpha.48",
  • "@storybook/react": "6.4.19",

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
Copy link

mel-miller commented May 20, 2022

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
Copy link
Contributor

Pickra commented May 21, 2022

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

@mel-miller
Copy link

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

@andapop
Copy link

andapop commented Jul 19, 2022

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

+1

@franktopel
Copy link

franktopel commented Aug 24, 2022

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
Copy link

alicegherbison commented May 8, 2024

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests