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

Add Multistory and Preview Decorator Support #1394

Closed
wants to merge 14 commits into from
Closed

Conversation

usulpro
Copy link
Member

@usulpro usulpro commented Jul 1, 2017

Issue: #1335

What I did

Add simple API for switching storyKinds to the Multistory Mode by adding : to storyKind

example:

storiesOf('Buttons:', module)
  .add('normal small', ...)
  .add('normal big', ...)
  .add('normal enormous', ...)

will show all stories on one preview page. All stories still are selectable in the Stories Panel.
You can use decorators to customize how each story will look.
It works on any hierarchy level as well:

storiesOf('Buttons:.normal', module)
  .add('normal small', ...)
  .add('normal big', ...)
  .add('normal enormous', ...);

storiesOf('Buttons:.accent', module)
  .add('accent small', ...)
  .add('accent big', ...)
  .add('accent enormous', ...);

will create a common Buttons page with both normal and accent buttons

How to test

Github Page: https://github.com/UsulPro/storybook-multistory

run cra-kitchen-sink example

@codecov
Copy link

codecov bot commented Jul 1, 2017

Codecov Report

Merging #1394 into master will decrease coverage by 14.35%.
The diff coverage is 54.54%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1394       +/-   ##
===========================================
- Coverage   35.74%   21.38%   -14.36%     
===========================================
  Files         472      236      -236     
  Lines       10134     5218     -4916     
  Branches     1196      719      -477     
===========================================
- Hits         3622     1116     -2506     
+ Misses       5784     3575     -2209     
+ Partials      728      527      -201
Impacted Files Coverage Δ
addons/options/src/preview/index.js 0% <0%> (ø) ⬆️
app/react/src/client/preview/index.js 0% <0%> (ø) ⬆️
addons/options/preview.js 0% <0%> (-100%) ⬇️
app/react/src/client/preview/render.js 80% <70.58%> (+80%) ⬆️
...ts/stories/required_with_context/Button.stories.js 0% <0%> (-100%) ⬇️
...dons/storyshots/stories/directly_required/index.js 0% <0%> (-100%) ⬇️
addons/links/src/preview.js 0% <0%> (-100%) ⬇️
app/react/demo.js 0% <0%> (-100%) ⬇️
addons/storyshots/src/storybook-channel-mock.js 0% <0%> (-100%) ⬇️
...s/stories/required_with_context/Welcome.stories.js 0% <0%> (-100%) ⬇️
... and 489 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d0d6e4f...52483bd. Read the comment docs.

@ndelangen
Copy link
Member

Awesome work @usulpro !

@usulpro
Copy link
Member Author

usulpro commented Jul 2, 2017

image

@usulpro
Copy link
Member Author

usulpro commented Jul 2, 2017

I added two options:

setOptions({
  multistorySeparator: /:/,
  previewDecorator: tileDecorator,
});

The first one sets the reg.exp. to determine multistories

The second allows to pass custom React Component to the root of preview:

const tileDecorator = stories =>
  <div>
    <h1>Button Tiles</h1>
    <div
      style={{
        display: 'flex',
        flexWrap: 'wrap',
        alignItems: 'stretch',
      }}
    >
      {stories}
    </div>
  </div>;

how to test:

Run kitchen-sink. Check out Buttons Guide -> tile: section

@usulpro
Copy link
Member Author

usulpro commented Jul 3, 2017

I guess it's ready!

@ndelangen @shilman @igor-dv could you review?

},
"dependencies": {
"@storybook/addon-actions": "^3.1.6",
"@storybook/addon-links": "^3.1.6",
"@storybook/addons": "^3.1.6",
"@storybook/addon-options": "^3.1.6",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm missing the point, so fix me if I'm wrong.
You are adding here a dependency for the addon-options -> hence a coupling to it. AFAIK addon-actions and addon-links are already deprecated as a default addons of the app. So it makes me unsure regarding this design.. (After that you will need to add it to app/react-native and app/vue )

Copy link
Member Author

@usulpro usulpro Jul 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@igor-dv, it's a very good question!

addon-options is a very special package - it doesn't provide any additional functionality like actions and lilnks. We use it to get access to UI API.

So maybe better to think that it's not an addon at all!? 🤔

For Storybook users it's just one more kind of API. Common use:

import { configure, storiesOf } from '@storybook/react';
import { setOptions } from '@storybook/addon-options'; // why I need one more package?

Nevertheless, the only my reason here is to provide such API:

setOptions({
  hierarchySeparator: /\/|:\//, // regular expression to separate stories nesting
  multistorySeparator: /:/, // regular expression to separate multistories section
  previewDecorator: stories => <div>{stories}</div>, // root decorator for preview
});

I guess it's much more comfortable for consumers to have all similar APIs in one place, than for example:

import { setOptions } from '@storybook/addon-options';
import { setPreferences } from '@storybook/react';

setOptions({
  hierarchySeparator: /\/|:\//, // regular expression to separate stories nesting
});

setPreferences({
  multistorySeparator: /:/, // regular expression to separate multistories section
  previewDecorator: stories => <div>{stories}</div>, // root decorator for preview
});

The last example is more consistent with the project design, but it can :suspect: >> 💥 >> :feelsgood:

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW i agree with @igor-dv that addon-options is a strange package and we should consider moving the functionality into addons. I've opened an issue #1430

I believe this is complementary to my proposal lower down to create a storiesOf(...).setOptions({ ... }) method for configuring showing multiple preview windows.

@usulpro usulpro changed the title Add Multistory Support Add Multistory and Preview Decorator Support Jul 6, 2017
Copy link
Member

@igor-dv igor-dv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it worth to open a discussion regarding the addon-option. If it's not an addon it should be somewhere else =)

@@ -7,6 +7,19 @@ export function init() {
// NOTE nothing to do here
}

let previewOptions = () => {};

function regExpStringify(exp) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool idea =)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe it worth to submit a separate PR with this feature to have it before we finish with this PR? @igor-dv is it still actual?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, missed this comment.. I think it's relevant..

@@ -1,4 +1,5 @@
/* global document */
/* eslint react/prop-types: 0 */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why ?

`,
};
return renderError(error);
// const element = multiStoriesSeparator
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

redundant comment ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure!

@ndelangen
Copy link
Member

ndelangen commented Jul 7, 2017

I love this feature, but the API, I'm not a fan of. I have reservations about using the first argument (string).

I do not want to end up supporting tons of options, settings, arguments, presets, loaders, methods inside a string.

I propose (discussed with @shilman) an generic API for setting options on a story:

storiesOf('a/b/c', module).setOptions({ previewMultiple: true })
  .add();

An option I'm personally also a fan of is to extend the second parameter to receive the options object. Possibly we could do both!

storiesOf('a/b/c', { module, previewMultiple: true })
  .add();

Copy link
Member

@ndelangen ndelangen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like the API to change

@shilman
Copy link
Member

shilman commented Jul 7, 2017

To add to @ndelangen 's comment, setOptions({ ... }) globally can behave like addDecorator to set options in the global scope, and setOptions on a story kind can add those options to the kind or the stories within the kind.

Still discussing this with @ndelangen @igor-dv @tmeasday @alexandrebodin and no consensus except that everybody is concerned about over-using the string argument.

@shilman shilman added this to the v3.3.0 milestone Jul 22, 2017
@usulpro usulpro mentioned this pull request Aug 26, 2017
3 tasks
@shilman shilman mentioned this pull request Sep 6, 2017
3 tasks
@danielduan danielduan modified the milestones: v3.3.0, v3.4.0 Nov 2, 2017
@ndelangen
Copy link
Member

@usulpro Want to pair and finish this?

# Conflicts:
#	addons/options/README.md
#	addons/options/src/preview/index.js
#	app/react/package.json
#	app/react/src/client/preview/render.js
#	examples/cra-kitchen-sink/.storybook/config.js
#	examples/cra-kitchen-sink/src/stories/index.js
#	examples/test-cra/package.json
#	lib/ui/README.md
@ndelangen
Copy link
Member

I Merged in release/3.3 and fixed it up a bit.

Some code is disabled.. but it seems to work.
I'm still feeling the : syntax is not that great. But I'm also ready to put some effort into this to get it ready for 3.3 release.

@shilman ?

@stale
Copy link

stale bot commented Dec 19, 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 stale bot added the inactive label Dec 19, 2017
@usulpro usulpro removed the inactive label Dec 20, 2017
@Hypnosphi Hypnosphi changed the base branch from release/3.3 to master December 23, 2017 23:54
@InkeyesLT
Copy link

Hey, very nice work on this feature, can we expect it any time soon?

@usulpro
Copy link
Member Author

usulpro commented Jan 30, 2018

@InkeyesLT Thanks! Actually, I was a bit busy for some time, but I hope I can come back to it.

@ndelangen if your offer is still relevant, it would be great if we make this happen together :) I described different possible API in my previous comment and here. Anyway, I'm open to discuss and continue this anytime when you're ready. Maybe next week?

@shilman shilman modified the milestones: v3.4.0, v4.0.0 Feb 3, 2018
@stale
Copy link

stale bot commented Mar 20, 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!

@henningmu
Copy link

Hi @ndelangen and @usulpro could you please tell me what's up with this PR / feature? Will it never be part of storybook or is development continued elsewhere?

@ndelangen
Copy link
Member

@henningmu This WILL be part of storybook in the future, but this PR has been open for too long, the codebase has changed too much. And the API proposed wasn't really satisfactory to the whole team.

I'm working on a project that will introduce this feature, but it will take a bit of time to complete.

@Hypnosphi Hypnosphi deleted the add-multistory-kind branch April 26, 2018 12:17
@mariepw
Copy link

mariepw commented Mar 18, 2019

@usulpro @henningmu @ndelangen FYI I was looking for this feature, as "workaround" I just created an overview page for all my stories :

import * as React from "react";
import { storiesOf, getStorybook } from "@storybook/react";

const allStories = getStorybook();

const AllStories = () => (
  <>
    {
      allStories.map((stories) => {
        return <> {
          stories.stories.map((story) => {
            return <>{ story.render() }</>
          })
        } </>
      })
    }
  </>
);

storiesOf("Overview", module).add("all", () => <AllStories />); 

Using Storybook 5.0.0

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

Successfully merging this pull request may close these issues.

10 participants