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

Composition: MDX pages don't get the appropriate Sidebar list items #11302

Closed
domyen opened this issue Jun 24, 2020 · 20 comments
Closed

Composition: MDX pages don't get the appropriate Sidebar list items #11302

domyen opened this issue Jun 24, 2020 · 20 comments

Comments

@domyen
Copy link
Member

domyen commented Jun 24, 2020

Describe the bug
I'm composing Storybook Design System into LearnStorybook's own Storybook. However the sidebar UI of the composed Storybook does not match SDS when it should.

Composed Storybook in LSB Original Storybook in SDS
image image

To Reproduce
Steps to reproduce the behavior:

  1. Go to Storybook Design System that's published online
  2. Look at the sidebar. Specifically, the yellow "document" icons for the DocPages
  3. Now go to LearnStorybook's own Storybook published online
  4. See that these aren't the same. When SDS is composed, we show the DocPages as regular stories instead of pages.

Expected behavior
I expect to see the same sidebar UI when I compose an external Storybook. DocPages should be rendered as Docpages.

shilman ndelangen do you know why this would happen?

@ndelangen
Copy link
Member

Oof, I'm not sure, will have to investigate!

@ndelangen ndelangen added this to the 6.0 milestone Jul 3, 2020
@ndelangen ndelangen self-assigned this Jul 3, 2020
@shilman shilman modified the milestones: 6.0, 6.0 composition Jul 3, 2020
@ndelangen
Copy link
Member

@shilman would you be able to be on a call tomorrow with me (my morning) dot see what is causing this diff:
Screenshot 2020-07-13 at 17 57 35
Screenshot 2020-07-13 at 17 57 41

I checked the generated json a bit, and I'm guessing it's some parameter | arg | global | config diff. But I'm not sure. Would love your assist.

@shilman
Copy link
Member

shilman commented Jul 13, 2020

@ndelangen i'm guessing this has to do with parameter normalization, where global parameters are now stored separately from component/story parameters, but i'd be happy to look at this together.

@ndelangen
Copy link
Member

Screenshot 2020-07-13 at 18 20 42

@shilman
Copy link
Member

shilman commented Jul 14, 2020

@tmeasday do you know what's up with this ☝️

@tmeasday
Copy link
Member

@ndelangen initially when you load the outer storybook and it grabs the /stories.json there are no docs-only stories listed as we currently filter them out in extract() and thus Chromatic doesn't get them, so they don't end up in there. We need to figure that out on the Chromatic side I suppose.

As for the second problem (what happens when you browse to a story) I don't think it is likely anything to do with normalization. AFAICT the display in the LHS is fairly simply associated with parameters.docsOnly which is set at the story level:

image

I can't see why it wouldn't be the same SET_STORIES event emitted in both cases (embedded vs browsed to directly). I can't see any possible way for them to be different. I would expect the handling in the refs code is likely what's going on there.

@ndelangen
Copy link
Member

Thanks to @shilman I now know how to fix 1 of these 2 issues:
Once the page type stories are added, they are still rendered incorrectly:
Screenshot 2020-07-14 at 13 01 09

@ndelangen
Copy link
Member

ndelangen commented Jul 14, 2020

To solve the other problem, we should probably start using this:

getDataForManager = () => {
return {
v: 2,
globalParameters: this._globalMetadata.parameters,
globals: this._globals,
error: this.getError(),
kindParameters: mapValues(this._kinds, (metadata) => metadata.parameters),
stories: this.extract({ includeDocsOnly: true, normalizeParameters: true }),
};

over raw .extract(), WDYT @shilman @tmeasday ?

I assume we use .extract() in chromatic somewhere? @ghengeveld ?

@ndelangen
Copy link
Member

The above should also be changed in the sb extract command

@shilman
Copy link
Member

shilman commented Jul 14, 2020

@ndelangen I think that makes sense. The reason I didn't implement this for more different consumers (e.g. storyshots, chromatic) was because i didn't understand the impact and didn't want to break anything. and i still don't understand the impact, but i think it sounds reasonable provided the calling code is updated to deal with those docsOnly stories.

@shilman
Copy link
Member

shilman commented Jul 15, 2020

Whoopee!! I just released https://github.com/storybookjs/storybook/releases/tag/v6.0.0-rc.5 containing PR #11537 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.

@shilman
Copy link
Member

shilman commented Jul 21, 2020

Jeepers creepers!! I just released https://github.com/storybookjs/storybook/releases/tag/v6.0.0-rc.13 containing PR #11584 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.

@shilman shilman closed this as completed Jul 21, 2020
@hipstersmoothie
Copy link
Contributor

I'm still seeing this issue on 6.1.x

@hipstersmoothie
Copy link
Contributor

MDX only stories from the storybook work but not from the composed storybook

Screen Shot 2020-12-01 at 4 34 14 PM

@shilman shilman reopened this Dec 2, 2020
@shilman
Copy link
Member

shilman commented Dec 2, 2020

@ndelangen @ghengeveld did this regress with the sidebar refactor?

@shilman shilman added the P0 label Dec 2, 2020
@shilman shilman added P1 and removed P1 labels Dec 2, 2020
@shilman shilman modified the milestones: 6.0 composition, 6.1.x Dec 2, 2020
@stale
Copy link

stale bot commented Dec 25, 2020

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

@stale stale bot added the inactive label Dec 25, 2020
@shilman shilman modified the milestones: 6.1.x, 6.2 candidates Mar 3, 2021
@stale stale bot removed the inactive label Mar 3, 2021
@shilman shilman removed this from the 6.2 candidates milestone Mar 11, 2021
@shilman shilman removed the tracked label Mar 30, 2021
@shilman shilman added this to the 6.3 improvements milestone Apr 1, 2021
@shilman
Copy link
Member

shilman commented Apr 1, 2021

We want to address this in 6.3. If you want to contribute to Storybook, we'll prioritize PR review for any fixes here. And if you'd like any help getting started, please jump into the #support channel on our Discord: https://discord.gg/storybook

@zavlalayan
Copy link

Hi,
I did a small research and found out that in Sidebar component the collapseDocsOnlyStories method removes leaf items that have only docs. But it is called only on storybook_internal stories, not the ref ones.
It causes the issue that in storybook composition there are 'Page' items.

@ndelangen
Copy link
Member

@zavlalayan can you open a PR with a fix?

@atflick
Copy link

atflick commented Aug 13, 2021

Not sure if this is related, but when I am developing I get links to all stories on a page and when it is built and deployed the sidebar only shows links to the pages. Making you scroll down to what you want vs. having the nice jump links. I'm using mdx stories

when developing:
image

this is built:
image

this is what my Forms.stories.mdx looks like essentially:

<Meta title="Styleguide/Forms" />

# Forms
---

## Inputs
<Story name='Inputs'>
  {{
    components: { Inputs },
    template: '<Inputs />'
  }}
</Story>

## Selects
<Story name='Selects'>
  {{
    components: { Selects },
    template: '<Selects />'
  }}
</Story>

## Checkboxes
<Story name='Checkboxes'>
  {{
    components: { Checkboxes },
    template: '<Checkboxes />'
  }}
</Story>

## Radios
<Story name='Radios'>
  {{
    components: { Radios },
    template: '<Radios />'
  }}
</Story>

## Layout
Vertical layout is preferred and should be the standard, however there are times when horizontal may make more sense.

<Story name='Layout'>
  {{
    components: { Layout },
    template: '<Layout />'
  }}
</Story>

## Validation

<Story name='Validation'>
  {{
    components: { Validation },
    template: '<Validation />'
  }}
</Story>

@shilman shilman removed this from the 6.4 improvements milestone May 20, 2022
@shilman shilman removed the P0 label Oct 18, 2022
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

7 participants