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

Allow passing options to storiesOf().add() as hook to customizing story behaviour in plugins #993

Closed
theinterned opened this issue May 4, 2017 · 18 comments

Comments

@theinterned
Copy link
Member

theinterned commented May 4, 2017

Having the generic ability to pass options alongside each story to the storiesOf and add method would open the door to some easier per-story customization in decorators or other add-ons.

I see few possible solutions:

Option 1: add an options argument

Make options an argument to storiesOf and add. This would look like this:

storiesOf('kind', kindOptions)
  .add('title', render, storyOptions);

Option 2: Make storiesOf and add accept a single options hash arg

Change the api for storiesOf and add so that they accept a single options arg. Something along the lines of:

storiesOf({title: 'kind title'})
  .add({
    title: 'story title', 
    render, 
    myOtherOption
  });

Note: Moved the JSX syntax proposal to #1006

@usulpro
Copy link
Member

usulpro commented May 5, 2017

Thank you @theinterned for opening this discussion

One more great variant (actually something between Options 1 and 2) was offered by @nutgaard in #705

storiesOf('Kind without metadata')
  .add('no-metadata', () => (<p>No metadata here</p>))
  .add('metadata', { story: () => (<p>No metadata here</p>), additionData: 'metadata' })

the big advantage of this is that we can pass data through addons like addon-info to ensure compatibility with them

@tmeasday
Copy link
Member

tmeasday commented May 5, 2017

Some thoughts:

  1. The idea of passing arbitrary options w/ a story seems good.
  2. I like the name "chapter" myself, especially to help with the nested problem, although I wonder if we should just adopt a more generic describe/it terminology too.
  3. I don't really understand why the JSX syntax helps? Also doesn't the story part need to be a function: how would that work in JSX?

@theinterned
Copy link
Member Author

theinterned commented May 5, 2017

@tmeasday in answer to your third question, I guess the big advantages that a JSX approach gives are the same familiar advantages that all react development gives:

  1. Familiarity: Storybook is a react tool so devs are familiar with the component style. There are a lot of use-cases that are solvable with simple and familiar patterns.
  2. Composition: This to me is the big one — components give you great flexibility to compose with sibling and child relationships.
  3. Replaceability: Any component could replace any other component with the same interface (like in the Liskov sense). Imagine being able to customize how how a Story works by just wrapping / decorating it or requiring an alternate Story from a different package.
  4. Consistent handling of arbitrary props: PropTypes, default props, props spreading ... all the features that react has for managing the complexity of passing around arbitrary props arguments.
  5. There are probably a lot of other reasons. These are the top four for me ...

@theinterned
Copy link
Member Author

@tmeasday as to your question "doesn't the story part need to be a function: how would that work in JSX?" Can you clarify that a bit? Aren't components functions (and sometimes classes)? or do you mean something else?

@tmeasday
Copy link
Member

tmeasday commented May 7, 2017

On the function part: I mean that the current interface expects to be passed a function that returns a React element (I guess equivalent to a stateless functional component). Your version above passes an element as a child to the Story component. (in the OO sense, it passes an "instance" rather than a "class").

The equivalent thing would be to pass in a prop: <Story render={() => <MyComponent ... />} />.

I'm not sure exactly why the interface is as it is, a guess is so that you get a new element each time you visit the story.

@tmeasday
Copy link
Member

tmeasday commented May 7, 2017

On the general question of a "react" interface, let's unpack this a little more. I suspect you can get a lot of the advantages you've stated with a basic OO class-based interface. I'm not necessarily advocating for that, but I want to use it for comparison. So something like:

new Chapter({
  title: 'Chapter',
  stories: [
    new Story({
      title: 'Story',
      render() { return <My Component ... />; },
    }),
});

To your points:

  1. I think people are similarly used to a functional or OO style.
  2. I don't really understand the composition point; can you give an example of what you mean?
  3. Classes are certainly easily extendable and replaceable.
  4. Consistent handling of props: What does React do here that helps? Isn't JSX just a thin wrapper around basic JS? Can you help me with an example?

I'm not necessarily against it but I am trying to tease it out so bear with me. I guess I just wonder if the "rendering" interface is restrictive for little benefit. For instance, it no longer becomes easy to take an instance of a chapter and get its stories (compare the two cases: a class instance with a stories property to an Chapter element with a set of children, some of which are Story elements), or easily change the way a chapter/story is rendered.

Is there a reason why restricting the way we think about stories to "rendering" is a good thing? I'm not sure there is.

@theinterned
Copy link
Member Author

@tmeasday I agree that you could absolutely describe any interface that is currently described in JSX in pure JS: that's one of the fundamentaly great things about JSX: it transpiles to just pure JS, but is so much nicer to look at then a deeply nested tree of objects and arrays.

So yes people are familiar with OO style and with, say, the functional style of most other testing packages (which I would sort of prefer to see somehow then the fluent api of storybooks as it stands now). But there's a certain expressiveness to the JSX syntax.

I think the value of composition is well expressed by @usulpro in slack: it makes it easy to solve problems like this: #766 because the add-on could be a component rather than a plugin.

@theinterned
Copy link
Member Author

I created a separate issue about the JSX syntax: #1006

we can continue the discussion there.

I feel that the JSX syntax may be a distraction from the conversation about a generic way to pass props to storiesOf and add which would be a very useful feature on it's own without a major overhall to the storybook api.

@ndelangen
Copy link
Member

I'm fully on board with this idea!

@ndelangen
Copy link
Member

Very solid proposal for #1209 api-v2

@shilman
Copy link
Member

shilman commented Jul 10, 2017

I'm board with Option 1. Any breaking change in the API had better be motivated by enabling something we can't do with the current API, IMHO.

@stale
Copy link

stale bot commented Oct 31, 2017

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. We do try to do some housekeeping every once in a while so inactive issues will get closed after 90 days. Thanks!

@stale stale bot added the inactive label Oct 31, 2017
@ndelangen ndelangen removed the inactive label Nov 1, 2017
@ndelangen ndelangen self-assigned this Nov 1, 2017
@stale
Copy link

stale bot commented Dec 16, 2017

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 60 days. Thanks!

@stale
Copy link

stale bot commented Feb 1, 2018

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 60 days. Thanks!

@stale stale bot added the inactive label Feb 1, 2018
@tmeasday
Copy link
Member

tmeasday commented Feb 1, 2018

I am working on this actually #2679!

@stale stale bot removed the inactive label Feb 1, 2018
@stale
Copy link

stale bot commented Mar 18, 2018

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 60 days. Thanks!

@stale stale bot added the inactive label Mar 18, 2018
@tmeasday
Copy link
Member

This is done actually, just waiting to be merged in #2679.

@stale stale bot removed the inactive label Mar 18, 2018
@Hypnosphi
Copy link
Member

Released as 4.0.0-alpha.0

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