storybookjs / storybook

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

Addon-backgrounds MDX support #14322

Open shilman opened 3 years ago

shilman commented 3 years ago

Hi @yannbf ,

Your PR fixes the problem for DocsPage generated documentation, but not for manually written MDX stories in the style described here.

The code in the PR tries to set styles on the CSS selector here

This works for DocsPage since it generates Anchor components for every story. However this pattern is not documented in the MDX guide above, and actually seems to conflict with the behaviour of the Meta tag here, which generates a single Anchor component at the top of the page for the first Story encountered (which is a bit strange tbh).

Would it be possible to make the background selection work on any story regardless of whether they are contained in an anchor?

Originally posted by @angryzor in https://github.com/storybookjs/storybook/issues/7978#issuecomment-726797915

yannbf commented 3 years ago

@patricklafrance we spoke about this back then. Did you have any ideas on how to best approach this? The addon must know what target to set a custom background for.

havercake commented 3 years ago

Hi everyone, for Stories that are wrapped in a Canvas element there is a an easy target class of:

sbdocs sbdocs-preview

You could set the background on that. (Note that this is div that the background colour is being set on by default already)

havercake commented 3 years ago

As @angryzor pointed out in their investigation (thanks for that), the change in the PR here.

If I understand correctly this line:

context.viewMode === 'docs' ? '#anchor--${context.id} .docs-story' : '.sb-show-main';

Could be changed to:

context.viewMode === 'docs' ? '.sbdocs-preview .docs-story' : '.sb-show-main';

This should work for both DocsPage generated stories and custom MDX stories (as long as they are inside a canvas)

yannbf commented 3 years ago

Hey @havercake thanks for your insights! The problem with that approach is that we lose the specificity of the story. That will indeed make the backgrounds work, but it will apply the same background to every story, even if each story has a backgrounds override.

Essentially this can only be solved if we change the structure of a docs block. I will see what I can do about it!

yannbf commented 3 years ago

Context

Hey @shilman, if all stories from docs used DocsStory instead of Story block, the addon would work just fine (as well as any other addon that needs specific story targeting). The reason is because the addons need a wrapper containing an identifier for that story, and when users write stories like this, it won't have the id in the wrapper:

<Canvas>
  <Story id="some-story"/>
</Canvas>

whereas the automatically generated docs contain the following structure:

<Anchor storyId={id}> // <-- Necessary piece!
  {subheading && <Subheading>{subheading}</Subheading>}
  {description && <Description markdown={description} />}
  <Canvas withToolbar={withToolbar}>
    <Story id={id} />
  </Canvas>
</Anchor>

Small POC

Here's a hack to make the addon work with custom MDX. In docs/src/blocks/Canvas.tsx:

image

And the result: 2021-03-26 at 18 31 17 - Lavender Wildebeest

This requires no change in the backgrounds addon, and it will work fine with automatically generated docs pages as well.. but I'm not happy with that. I just wanted to show a mechanism we need in order to make addons like backgrounds work in docs. Ideally both auto generated docs and mdx docs stories should have a similar structure, so that addons could work consistently across all supported formats.

As reference, here's the MDX used in that test

import {
  Story,
  Canvas,
  Meta,
} from '@storybook/addon-docs/blocks';
import BaseButton from '../components/BaseButton';

<Meta
  title="Addons/Backgrounds/mdx"
  id="addon-backgrounds-mdx"
  component={BaseButton}
  parameters={{ 
    backgrounds: {
      default: 'dark',
      values: [
        { name: 'white', value: '#ffffff' },
        { name: 'light', value: '#eeeeee' },
        { name: 'gray', value: '#cccccc' },
        { name: 'dark', value: '#222222' },
        { name: 'black', value: '#000000' },
      ]
    }
  }}
/>

# Story from id

This will use the story id as target for backgrounds 

<Canvas>
  <Story id="addons-backgrounds-canvas--overridden" />
</Canvas>

# Multiple stories from id inside canvas

This will use the first story id as target for backgrounds 

<Canvas>
  <Story id="addons-backgrounds-canvas--with-gradient" />
  <Story id="addons-backgrounds-canvas--story-1" />
</Canvas>

# Story defined in mdx

This will use the meta id as target for backgrounds, thus using default backgrounds from meta parameters

<Canvas>
  <Story name="simple story">
    <button>non story</button>
  </Story>
</Canvas>
rbardini commented 3 years ago

Any updates on this? The different structures between regular and MDX stories in docs mode make it difficult to maintain certain addons that should work across formats—I've even noticed the anchor disappearing while switching between canvas and docs tabs 😕

lazenyuk-dmitry commented 1 year ago

It is strange why this problem still does not have a solution.

@yannbf said everything correctly. The problem is the absence of a wrapper around <Canvas> when using it separately from <Storis> in MDX.

The way to wrap <Canvas> in <Anchor> is very inconvenient. Since you have to manually prescribe storyId every time. If <Anchor> was able to put his storyId himself)

<Anchor storyId={id}>
    <Canvas />
</Anchor>

I can only share a small temporary fix.

Add the next code to your preview.js:

decorators: [
        (story, context) => {
            const storyAnchor = `anchor--${context.id}`;
            const existAnchor = context.canvasElement.closest(`#${storyAnchor}`);
            const storyContainer = context.canvasElement.closest(".sbdocs");

            if (!existAnchor && storyContainer) {
                storyContainer.id = storyAnchor;
            }

            return {
                components: { story },
                template: `<story />`,
            }
        },
    ],

I use Vue, in your case return will be different.

yannbf commented 1 year ago

Can anyone confirm whether this is still applicable in Storybook 7? Both the bug and the workaround mentioned in this discussion? Thank you!

ClementChaumel commented 10 months ago

The bug is still a thing for me on 7.1.0 I haven't been able to get the workaround to work though.

pimenovoleg commented 6 months ago

Yes it still doesn't work with 7.6 Repo: https://github.com/radix-ng/primitives/tree/main/packages/primitives/.storybook

https://661288c24c1e1cf776635f22-kusmqcjumr.chromatic.com/?path=/docs/primitives-separator--docs

Do you have any plans to fix this? Or is it already fixed in version 8?

pimenovoleg commented 6 months ago

for Angular, you can work around this as:

decorators: [
        (Story, context) => {
            const storyAnchor = `anchor--${context.id}`;
            const existAnchor = context.canvasElement.closest(`#${storyAnchor}`);
            const storyContainer = context.canvasElement.closest('.sbdocs');

            if (!existAnchor && storyContainer) {
                storyContainer.id = storyAnchor;
            }

            return Story(context);
        }
    ],

Angular 17.x Sb 7.6.x