storybookjs / storybook

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

Addon-docs: Show source block with generated knob values #8342

Closed JamesIves closed 4 years ago

JamesIves commented 5 years ago

Describe the bug

I'm using addon-knobs along with addon-docs, and I'm having a hard time getting the two to work well together.

Within my Component.stories.mdx file I have the following:

export const defaultProps = (selected, icon) => ({
  selected: boolean('selected', false),
  label: text('label', 'Essentials'),
  description: text(
    'description',
    'This s a description.'
  ),
});

<Preview>
  <Story name="default">
    <Box
      {...defaultProps(false)}
    />
  </Story>
</Preview>

I'm using defaultProps as a helper function for all of my stories in the mdx file as they all share the same props. The problem I have is that when this renders in the Storybook docs page it shows the function call instead of all of the props.

Screen Shot 2019-10-08 at 10 47 56 AM

If i don't use a function to generate these, I still see the addon-knobs function call instead of the actual prop value.

Screen Shot 2019-10-08 at 10 48 55 AM

To Reproduce

  1. Create a mdx story with addon-knobs.
  2. Observe that any created story ends up having the addon-knobs function call instead of the value the functions return.

Expected behavior

I'd expect it to show the actual prop values. For example:

<Box selected={true} label="The label" />

Is there a correct way to do this? With the way it works currently it's hard to define stories inside of the mdx file exclusively.

JamesIves commented 5 years ago

I've also experienced some inconsistencies with getting the default knob values to apply to stories that are created through the mdx format.

For instance the following code should apply the selected prop as false In the first occurrence, and in the second one as true.

export const defaultProps = (selected) => ({
  selected: boolean('selected', selected),
  label: text('label', 'Essentials'),
  description: text(
    'description',
    'These are the basics: paying the bills and putting food on the table. They include your non-discretionary spending.'
  ),
});

<Preview>
  <Story name="default">
    <Box {...defaultProps(false)}/>
  </Story>
</Preview>

<Preview>
  <Story name="selected">
    <Box {...defaultProps(true)}  />
  </Story>
</Preview>

In this scenario, whatever the first defaultProps() function call returns is what gets applied to all stories. This makes creating several staged stories difficult when there's a lot of shared props.

Doing the following pattern does appear to work correctly, so I'm wondering if it's somewhere in the addon-knobs addon.

export const myName = (selected) => ({selected})

<Preview>
  <Story name="default">
    <Box {...myName(false)}/>
  </Story>
</Preview>

<Preview>
  <Story name="selected">
    <Box {...myName(true)}  />
  </Story>
</Preview>

If there's a workaround, or a potential starting point to work on a fix I'd be happy to take a look.

JamesIves commented 5 years ago

Let me know if I should make a separate issue for the problem above @shilman ☝️

shilman commented 5 years ago

Thanks, we can keep them lumped together for now!

kkorach commented 5 years ago

I'm seeing the same thing with CSF stories. If the prop is set by a knob then it looks like its using the component's default value instead of what the knob says.

mrmckeb commented 5 years ago

Adding a groupId (third parameter) to knobs solved this for me.

Each should be unique.

text('Text', basic, 'basic')

JamesIves commented 5 years ago

Adding a groupId (third parameter) to knobs solved this for me.

Each should be unique.

text('Text', basic, 'basic')

Which version of knobs are you using? This workaround doesn't seem to work for me. In the Canvas view it looks correct, but not in the Docs view.

mrmckeb commented 5 years ago

Everything is at latest stable (5.2.4). This could be related to something else though in your case.

What version are you on?

JamesIves commented 5 years ago

The same version that you're on. I can try and get a working example soon, we've scraped the use of addon-docs until this can get figured out.

joelstransky commented 5 years ago

Just include withKnobs in the Meta decorators and knobs work same as they do in CSF. <Meta title="Desktop|Image Gallery" component={ImageGallery} decorators={[withKnobs]} />

stale[bot] commented 4 years ago

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!

daneah commented 4 years ago

I was inquiring about roughly this issue in Discord the other night—it appears that knobs with the same label act something like singletons in Docs mode, and the first default value that's defined is used for all components in the Docs tab. Adding a groupId is a nice idea and worked for me just now on 5.3.0-beta.31.

stale[bot] commented 4 years ago

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!

daneah commented 4 years ago

This is still valuable to fix.

joelstransky commented 4 years ago

@daneah This might be better as a new ticket.

liamross commented 4 years ago

Yeah this is still broken on version 5.3.0-rc.12, even after adding groupId. I have a button component with some simple props and this is what I get in the docs "Show code" expandable:

() => (
<Button
  disabled={boolean('disabled', false, '_')}
  onClick={action('onClick')}
>
  {text('children', 'Button content', '_')}
</Button>
)

For reference, here's my Button.stories.tsx file:

import { action } from '@storybook/addon-actions';
import { boolean, text } from '@storybook/addon-knobs';
import React from 'react';
import Button from '../Button';
import docs from './README.md';

export default { title: 'Button', component: Button, parameters: { info: docs } };

export const Basic = () => (
  <Button
    disabled={boolean('disabled', false, '_')}
    onClick={action('onClick')}
  >
    {text('children', 'Button content', '_')}
  </Button>
);

Also, while I'm here, I am wondering if there are any plans to allow you to set knobs within docs without having to go back to canvas? Between this issue and the knobs not being within docs, It is making me strongly consider removing story previews from docs entirely since they provide minimal value (non configurable and with broken code previews)

monospaced commented 4 years ago

Can also confirm what @liamross is saying. My approach has been to write new stories, without knobs, that speicfically target the docs page code previews. Then include a knobs story to be access by developers, in the canvas tab, that I exclude from the docs:

export const basic = () => (
  <Image alt="Kitten" src="http://placekitten.com/800/432" />
);

export const knobs = () => (
  <Image
    alt={text('alt', 'Kitten')}
    src={text('src', 'http://placekitten.com/800/432')}
  />
);

knobs.story = { parameters: { docs: { disable: true } } };

This works pretty well, but would not be necessary if the code previews showed the default values specifed by the knobs functions.

arfa commented 4 years ago

@monospaced

also this work

export default {
  title: 'Button/pros',
  component: Button,
  parameters: { docs: { disable: true } },
};
export const button: React.FC<ButtonProps> = () => <Button>{text('children', 'Button content', '_')}</Button>;
monospaced commented 4 years ago

@arfa yep, that works if you want to exclude the whole file from docs, not just an individual story.

monospaced commented 4 years ago

@shilman nice one for the issue rename. "Show source block with generated knob values" is exactly what we need i think.

liamross commented 4 years ago

Just bringing up my question again, not sure if anyone has this information:

"I am wondering if there are any plans to allow you to set knobs within docs without having to go back to canvas?"

arfa commented 4 years ago

@arfa yep, that works if you want to exclude the whole file from docs, not just an individual story.

Yes That is what I am doing. Docs with description of the cases of my component and any other additional informations. And canvas with knobs to manipulate props.

arfa commented 4 years ago

Just bringing up my question again, not sure if anyone has this information:

"I am wondering if there are any plans to allow you to set knobs within docs without having to go back to canvas?"

I think it is a good idea to keep docs for documentation and isolate the canvas to manipulate the component. but may be one canvas for all the stories of the same component.

shilman commented 4 years ago

@liamross #6639 is coming, probably in a 6.x release.

joelstransky commented 4 years ago

I still don't see the point of having Knobs UI in Docs. Would it still be an add-on tab? Would it be one set of knobs that controls all the stories with Knob params or would a Knob UI need to be appended to every story? Why not include a few more stories showing the important variations? I'm not saying it's pointless, just doesn't seem like a high priority.

JamesIves commented 4 years ago

I still don't see the point of having Knobs UI in Docs. Would it still be an add-on tab? Would it be one set of knobs that controls all the stories with Knob params or would a Knob UI need to be appended to every story? Why not include a few more stories showing the important variations? I'm not saying it's pointless, just doesn't seem like a high priority.

For me at least It's not so much about making the knobs visible in the docs so much as it's making the previews that are generated as an outcome useful. Otherwise you end up with a bunch of component previews with business logic in them.

Originally we wanted to define all of our stories in our docs (still using Canvas for addons) but we had to move away from that approach in favor of the storiesOf api to make this work correctly. Now we define our stories in the story.js file, and we setup repeated examples in the .mdx file. Admittedly this works well, but it's not entirely future proofed I feel.

https://github.com/UnitedIncome/components/tree/develop/components/molecules/Cabinet

joelstransky commented 4 years ago

Otherwise you end up with a bunch of component previews with business logic in them.

Can you elaborate on this?

Originally we wanted to define all of our stories in our docs (still using Canvas for addons) but we had to move away from that approach in favor of the storiesOf api to make this work correctly. Now we define our stories in the story.js file, and we setup repeated examples in the .mdx file. Admittedly this works well, but it's not entirely future proofed I feel. https://github.com/UnitedIncome/components/tree/develop/components/molecules/Cabinet

Great looking code there btw. Maybe it's my level of experience with Storybook but I don't understand this. I define everything in a single MDX and get both a Canvas tab where I can play with add-ons and a Docs tab that renders my MDX in full. Is it that docs contaminate the canvas or visa versa? I can definitely understand wanting more control over content, just hasn't been a problem for me yet.

JamesIves commented 4 years ago

The biggest reason we have to separate the two out is mostly due to how the preview code gets generated. As knobs require a function the value gets rendered in the preview as that, instead of what the result of that function produces.

<Dropdown label={text('label', 'Default')} />

In our markdown file we generate document specific versions that have more context around why they are being displayed that don't use knobs to prevent this problem. In the example above I'd expect label to look like this:

<Dropdown label="Default" />

When you combine that with a helper function that generates a series of default props the code examples end up looking really messy. The same thing happens with the InVision integration:

<DollarCircleIllustration {...defaultProps(false, false, false)} />

I'd love for there to be an easy way to generate clean code previews as that will prevent a bunch of repetition on our end. I'd also love for there to be an easy way to generate stories that are defined in the .mdx file, but hidden from the Docs tab so they only appear in the canvas.

joelstransky commented 4 years ago

Thanks for elaborating! Yes, I hate seeing add-on code in the sample. Maybe storybook needs a babel extension. And yah, seems like a simple prop could flag a story as "Docs only". Similar to the OP's point of needing to be able to dictate if knobs should render controls next to MDX stories.

atanasster commented 4 years ago

@JamesIves and other participants - you make great points. We are adding to the csf story format, and with sb6+ the properties(knobs) will be created in a declarative way and without any addon code in the story, so this should address your main concerns. here's how you would write your knob'ed stories in 6.0:

export const selectPropStory = ({ label, color, backgroundColor }) => (
  <Button color={color} backgroundColor ={backgroundColor}>{label}</Button >
);

Additionally, we can add a second tab to the preview component to display the raw html code, maybe even a dom-type explorer for the story - what you think?

joelstransky commented 4 years ago

I certainly wouldn't mind a literal component. something like:

const knobs = {
  disabled: boolean("Disabled", false),
  label: text("Label", "Hello Storybook")
};
<Preview>
  <Story name="all checkboxes">
    <Knobs {...knobs}>
      {({ disabled, label }) => <button disabled={disabled}>{label}</button>}
    </Knobs>
  </Story>
</Preview>;
atanasster commented 4 years ago

Yes, that would be the goal, sth


<Preview>
  <Story name="all checkboxes" properties={knobs}>
      {({ disabled, label }) => <button disabled={disabled}>{label}</button>}
  </Story>
</Preview>;
shilman commented 4 years ago

Nothin' but ❤️ 's for this work @atanasster. Heroic! 🥇

JamesIves commented 4 years ago

Amazing! This will be a game changer for us!

atanasster commented 4 years ago

To whet your appetite:

csf stories with properties (aka knobs): https://github.com/atanasster/storybook/blob/properties-2/examples/official-storybook/stories/addon-controls/props-editors.stories.js

mdx stories with properties (aka knobs): https://github.com/atanasster/storybook/blob/properties-2/examples/official-storybook/stories/addon-controls/propeditors.stories.mdx

It should make it into the 6.0 alphas by mid-february and we would love to get testing and feedback for it, especially you guys have been at it for some time already.

and a screenshot of the source code preview for this story:

export const textDefaultProp = ({ text }) => text;
textDefaultProp.story = {
  properties: {
    text: 'Hello',
  },
};

grab42

dvnrsn commented 4 years ago

These pages are not found ^ but I'm trying to figure out how to get source code from knobs right now. A major roadblock in the DX for us. Happy to test and provide feedback

alexhhn commented 4 years ago

Is this supported in current latest release, or only in alpha version?

snyderhaus commented 4 years ago

Still no update on this? All those pages are not found, unfortunately.

goldhand commented 4 years ago

Looks like the links @atanasster are to an experimental "addon-controls" addon that was removed. See the commit history in properties-2 branch.

The removed code is here: https://github.com/atanasster/storybook/tree/4a62e69ecb82fbfc19c65c834309a93e50de0fe7/examples/official-storybook/stories/addon-controls

I don't know what the plans are for this but it sounds nice :)

Side note: It would be amazing if the knobs / controls could be generated from prop types, similar to how the props table is.

shilman commented 4 years ago

@goldhand It's work in progress. More info here: https://docs.google.com/document/d/1Mhp1UFRCKCsN8pjlfPdz8ZdisgjNXeMXpXvGoALjxYM/edit?usp=sharing

f1yn commented 4 years ago

Contrarian here, I think this new prop injection is a terrible idea, namely since it basically obliterates the whole point of copy on source display components.

If a potential client's developer tried to copy and paste source code, they'd be under the impression that these props exist on our platform. For now I've been devising a workaround for this, but this feature might detract from the whole copy-paste formula some devs rely on for prototyping, especially for props that aren't easy digestible from a props table.

shilman commented 4 years ago

@flynnham you can still use no-arg functions in 6.0. in a future iteration of the source block we will show the live values in the code per this issue so you can copy and paste it.

Story:

const Basic = ({ label }) => <Button label={label} />

Raw display:

({ label }) => <Button label={label} />

Values display:

({ label }) => <Button label="actual label" />
f1yn commented 4 years ago

Because the storybook environment we're building atm, we've already been using a custom Source block in the documentation (which I've cleaned up to share). As a last resort to not confuse clients, I've opted for a very cheap static analysis version that's getting the job done. We're not using weird knob configurations so this is doing the job just fine, but for more advanced usages (like higher order functions and values dependant on closures), you'd need a full VM setup, or to setup this call within the same context as a call (which could be a callback hook).

Alternatively, you can detect variables also using static analysis and then add placeholders when evaluating (as shown below).

Edit: added Object serialization

import React, { useContext } from 'react';
import { DocsContext } from '@storybook/addon-docs/blocks';
import { Source } from '@storybook/components';

/**
 * Adapted from https://github.com/storybookjs/storybook/blob/master/addons/docs/src/blocks/Source.tsx
 */
const extractLines = (source, location) => {
    const { startBody: start, endBody: end } = location;
    const lines = source.split('\n');
    if (start.line === end.line) {
        return lines[start.line - 1].substring(start.col, end.col);
    }

    // NOTE: storysource locations are 1-based not 0-based!
    const startLine = lines[start.line - 1];
    const endLine = lines[end.line - 1];

    return [
        startLine.substring(start.col),
        ...lines.slice(start.line, end.line - 1),
        endLine.substring(0, end.col),
    ];
};

/**
 * Joins the original source for various
 * @param originalLines
 * @return {*}
 */
function applyKnobDefaults(originalLines) {
    const joinedSource = originalLines.join('\n');

    // Special cases for knobs for resolving index. Most knob calls use [1] as the defaultValue index
    const specialTypeToParamIndex = {
        select: 2,
        radios: 2,
        files: 2,
    };

    // safer and slightly eval for converting object into statement
    // see: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/eval#Do_not_ever_use_eval!
    const evaluateObject = (obj) => Function('"use strict";return (' + obj + ')')();

    // Special sauce regular expression, will extract the block group representing a common knob helper
    // This also works for multiline knobs
    return joinedSource
        .replace(/(color|boolean|text|array|select|object|radios|files)\(((\s|\S)*?)\)/g, (fullMatch, knobType, baseRawParams) => {
            // Super hack: Evaluate the parameter list as a native object. Note this won't respect the closure of
            // the original source. That would require a full VM OR to evaluate the whole script in isolation
            try {
                // Anything that we can't resolve (that's not a function call), attempt to replace with dummy value
                const rawParams = baseRawParams.replace(/([a-zA-Z0-9()]+?),/g, 'null,');
                const parsedParams = evaluateObject(`[${rawParams}]`);
                // Determine which index to use
                const indexForExtraction = specialTypeToParamIndex[knobType] || 1;
                const result = parsedParams[indexForExtraction];

                if (result && typeof result === 'object') {
                    // Adapted from https://stackoverflow.com/a/11233515
                    return JSON.stringify(result).replace(/"([^"]+)":/g, ' $1: ');
                }

                return JSON.stringify(result);
            } catch (error) {
                console.error('Failed to automatically resolve knob value', error);
                return fullMatch;
            }
        })
        .split('\n');
}

export function Example({
    id: customId,
}) {
    const context = useContext(DocsContext);

    const data = customId ?
        context.storyStore.fromId(customId) :
        context;

    const { source: rawSource } = data.parameters.storySource;
    const location = data.parameters.storySource.locationsMap[data.id];

    // Extract the source code for the contextual (or provided) story
    let lines = applyKnobDefaults(extractLines(rawSource, location));

    return (
        <Source
            code={lines.join('\n')}
            language="jsx"
            format={false}
        />
    )
}

It's not the prettiest solution, but just wanted to highlight that it is possible with static analysis too.

shilman commented 4 years ago

Hi gang, We’ve just released addon-controls in 6.0-beta!

Controls are portable, auto-generated knobs that are intended to replace addon-knobs long term.

Please upgrade and try them out today. Thanks for your help and support getting this stable for release!

shilman commented 4 years ago

For anybody who is interested in Controls but don't know where to start, I've created a quick & dirty step-by-step walkthrough to go from a fresh CRA project to a working demo. Check it out:

=> Storybook Controls w/ CRA & TypeScript

There are also some "knobs to controls" migration docs in the Controls README:

=> How do I migrate from addon-knobs?

AnnyCaroline commented 4 years ago

Loved the idea of use controls inside Docs page, but the code is still strange. From the Storybook "Controls w/ CRA & TypeScript" example:

image

shilman commented 4 years ago

@AnnyCaroline Yup. I'll probably implement the improved source rendering in 6.1 based on the controls work (NOT the knobs package, which will be deprecated at some point in the future), so I felt it was relevant to share the progress.

shilman commented 4 years ago

Great Caesar's ghost!! I just released https://github.com/storybookjs/storybook/releases/tag/v6.0.0-beta.38 containing PR #11332 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.