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 left panel hierarchy support #1329

Merged
merged 16 commits into from
Jun 25, 2017

Conversation

igor-dv
Copy link
Member

@igor-dv igor-dv commented Jun 21, 2017

Issue:
Substories/Hierarchy #151

What I did

I've added the option to visualize the "namespace syntax" of stories as a hierarchy in the stories panel (left panel).

For example, by providing stories like these:

storiesOf('web.component.Button', module)
  .add('One', ...)
  .add('Two', ...)
  .add('Three', ...)

storiesOf('web.component.Link', module)
  .add('One', ...)
  .add('Two', ...)
  .add('Three', ...)

The left panel will be like this:

web
|  component
|  |  Button
|  |      One    <-- if this is selected, other components are collapsed
|  |      Two
|  |      Three
|  |  Link

You'll be able to control it by defining the new "leftPanelHierarchy" prop of the uiOptions as a true (by default it's false).

How to test

You can run the storybook/lib/ui/example

@codecov
Copy link

codecov bot commented Jun 21, 2017

Codecov Report

Merging #1329 into 151-story-hierarchy will increase coverage by 0.9%.
The diff coverage is 65.34%.

Impacted file tree graph

@@                  Coverage Diff                   @@
##           151-story-hierarchy    #1329     +/-   ##
======================================================
+ Coverage                13.75%   14.66%   +0.9%     
======================================================
  Files                      202      203      +1     
  Lines                     4623     4713     +90     
  Branches                   616      512    -104     
======================================================
+ Hits                       636      691     +55     
- Misses                    3453     3591    +138     
+ Partials                   534      431    -103
Impacted Files Coverage Δ
addons/info/src/components/Story.js 0% <ø> (ø) ⬆️
lib/ui/src/modules/api/index.js 0% <0%> (ø) ⬆️
app/react-native/src/server/index.js 0% <0%> (ø) ⬆️
lib/ui/example/client/provider.js 0% <0%> (ø) ⬆️
...ui/src/modules/ui/components/left_panel/stories.js 100% <100%> (ø) ⬆️
...b/ui/src/modules/ui/components/left_panel/index.js 100% <100%> (ø) ⬆️
lib/ui/src/modules/ui/libs/hierarchy.js 46.66% <54.9%> (ø)
lib/ui/src/modules/ui/containers/left_panel.js 25.71% <57.14%> (+5.71%) ⬆️
addons/knobs/src/KnobManager.js 38.57% <0%> (ø) ⬆️
lib/ui/src/modules/ui/containers/down_panel.js 23.52% <0%> (ø) ⬆️
... and 37 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 152630b...21d7173. Read the comment docs.

@usulpro
Copy link
Member

usulpro commented Jun 21, 2017

@igor-dv Thanks for this implementation!
UI and API looks great to me! 👏

I played with this solution and found some issues:

  1. stories with hierarchy are located at the end of stories list even if I add them in the middle

  2. It creates two separate items in the stories list if I .add stories in the middle of hierarchy path. e.g.:

storiesOf('Atoms.Moleculs', module)
  .add('with text', () => <Button>Hello Button</Button>)
  .add('with some emoji', () => <Button>😀 😎 👍 💯</Button>)


storiesOf('Atoms.Moleculs.Cells', module)
  .add('with text2', () => <Button>Hello Button</Button>)
  .add('with some emoji2', () => <Button>😀 😎 👍 💯</Button>)

I've got such menu:
image

expected behavior: show only one item with stories and a substory item

  1. It doesn't work with Knobs. I tried to change some values - no response.

  2. I played with some nested levels and faced that in some cases it opens not the first item when I select the storyKind.

the way to reproduce
storiesOf('Atoms.Moleculs.Cells.simple', module)
  .add('with text', () => <Button>Hello Button</Button>)
  .add('with some emoji', () => <Button>😀 😎 👍 💯</Button>)

storiesOf('Atoms.Moleculs.Cells.more', module)
  .add('with text2', () => <Button>Hello Button</Button>)
  .add('with some emoji2', () => <Button>😀 😎 👍 💯</Button>)

storiesOf('Atoms.Moleculs', module)
  .add('with text', () => <Button>Hello Button</Button>)
  .add('with some emoji', () => <Button>😀 😎 👍 💯</Button>)

storiesOf('Atoms.Moleculs.Cells', module)
  .add('with text2', () => <Button>Hello Button</Button>)
  .add('with some emoji2', () => <Button>😀 😎 👍 💯</Button>)

When I click Atoms first time it opens all nested items this way:

image

possible it's overwhelming case thought :)

My suggestion

In general, I like this solution. In order to succeed with this feature don't you mind to collaborate with storybook-addon-chapters wich provides hierarchy as well but in another way? And possible with other developers who'd like to jump to this feature. So we can create a separate branch for this feature and continue the work.

Copy link
Member

@usulpro usulpro left a comment

Choose a reason for hiding this comment

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

see some issues above

@alexandrebodin
Copy link
Contributor

My bad I clicked the wrong button xD

@alexandrebodin
Copy link
Contributor

Full disclosure.

I'm working a moving the ui to redux instead of mantra. While doing this I am making a hierarchical view to. I think we should merge @igor-dv PR because my work is not ready yet.

I'll open a PR so @igor-dv can look at it and the kirks I had to handle to make the hierarchy work has I wanted (Hoping this will help you with fix bugs :))

If you want to have a look I'm opening a PR #1334

@igor-dv
Copy link
Member Author

igor-dv commented Jun 22, 2017

@usulpro , thanks for your review.

Regarding the issues:

  1. Fixed
  2. I don't know if it's a desired behavior . I assume that will be provided a "fully qualified" component name.
    For example in "Atoms.Molecules" , "Atoms" is a namespace and "Molecules" is a component name. In "Atoms.Molecules.Cells" - "Atoms.Molecules" is a namespace and "Cells" is component name..
    Would like to here more people regarding this, but maybe you are right
  3. I checked it with Knobs and it worked for me (added a few examples in cra-kitchen-sink)
  4. After some fixes it looks ok =)

Regarding the storybook-addon-chapters. I've considered it as an alternative for the projects in a company I work, but it didn't meet our needs.

  1. It always changes the context of the stories in the left panel
  2. It adds things to the preview section

In general, I think that the left panel should look like most of the IDEs files explorer (or how it's called..)

Though I am big fan of the plugins idea, Storybok addons api doesn't allow us to create a proper addon for the left panel..

Anyway, I think it's a great plugin, and would like to help with it.

After a bit UI changes, it looks like this now:
storybook_hierarchy

@shilman shilman changed the base branch from master to release/3.2 June 22, 2017 22:07
@igor-dv
Copy link
Member Author

igor-dv commented Jun 23, 2017

storybook_hierarchy_toggle

@shilman
Copy link
Member

shilman commented Jun 23, 2017

@igor-dv my guess is that "toggle story hierarchy" is gratuitous. My guess is that we release this as a backwards-compatible feature in 3.2 and then, assuming lots of people use it, we just make it the default in 4.0.

@shilman
Copy link
Member

shilman commented Jun 23, 2017

@igor-dv I think it would be nice to be able to pass a function that constructs the hierarchy from the story name. So by default that would be (storyName) => storyName.split(".") but that it could be overridden in the configuration.

Use cases:

  1. Sketch symbols use the naming convention a/b/c/SymbolName, and I've considered exporting these to Storybook as part of a design system
  2. I've also done things like kindName-storyName_deviceSize and would probably override the default in some collaboration-related features I've developed in private addons.

@igor-dv
Copy link
Member Author

igor-dv commented Jun 23, 2017

@shilman
Regarding the splitting - great idiea. I felt it's a bit hardcoded. So i can provide it as a parameter in the
uiOptions.leftPanelHierarchy

like by default it's a boolean - uiOptions.leftPanelHierarchy = false

but in a more expanded way it's

uiOptions.leftPanelHierarchy = {
  enabled: true,
  split: story => (storyName) => storyName.split(".")
}

WDYT ?

@shilman
Copy link
Member

shilman commented Jun 23, 2017

@igor-dv another idea (don't know if it's a good one):

Existing behavior (default):

uiOptions = {
  storyHierarchy: (storyName) => [storyName]
}

New behavior:

uiOptions = {
  storyHierarchy: (storyName) => storyName.split('.')
}

@alexandrebodin
Copy link
Contributor

I'm picky but I'm not sur about using 'Kind1.Kind2'
I would suggest a path like syntax 'Kind1/Kind2' to communicate the intent a bit more.

In my opinion . might be uses widely for any other meaning. and / is already used by users to have a visual clue of the separation. This would bring direct benefits to those already using this standard way of declaring URI don't you think ?

@shilman
Copy link
Member

shilman commented Jun 23, 2017

@alexandrebodin I don't have a strong opinion about this, but / would also be my preferred choice (see the Sketch comment above)

@igor-dv
Copy link
Member Author

igor-dv commented Jun 23, 2017

@shilman

uiOptions = {
  storyHierarchy: (storyName) => [storyName]
}

That works.. but toggling became harder to handle.. I need to add something to state..

@alexandrebodin
If we will provide the splitting function as an option, it will be easier to tweak this..
WDYT ?

@shilman
Copy link
Member

shilman commented Jun 23, 2017

@igor-dv why do we need toggling?

Also, this may be out of scope, but how hard would it be to replace the left panel with a generic tree widget, such as react-treebeard with custom styles and key-bindings.

I think Storybook's custom list UI kind of made sense when it was a flat list, but now that we have hierarchy, there are already common UI's for dealing with this that everybody (including non-engineers) are used to using.


// Atomic

storiesOf('Atoms.Molecules.Cells.simple', module)
Copy link
Member

Choose a reason for hiding this comment

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

I think the order is backwards: Cells.Molecules.Atoms?

Copy link
Member

Choose a reason for hiding this comment

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

possible we could get our existing kitchen-sink stuff and reorganize in a hierarchical way?

@@ -78,25 +78,62 @@ export default class ReactProvider extends Provider {
this.api = api;
this.api.setOptions({
name: 'REACT-STORYBOOK',
sortStoriesByKind: true,
Copy link
Member

Choose a reason for hiding this comment

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

"breaking"?

Copy link
Member Author

Choose a reason for hiding this comment

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

As far as I checked - no. But maybe it's already has another meaning.. Anyway the hierarchy is built after the filtering:
image

@igor-dv
Copy link
Member Author

igor-dv commented Jun 23, 2017

@shilman
I just thought toggling will be a nice thing, but you are right, I don't see benefits of having the old style view.

treebeard - interesting, I'll check it..

@igor-dv
Copy link
Member Author

igor-dv commented Jun 23, 2017

I think a better name will be

uiOptions = {
  resolveStoryHierarchyNamespace: (storyName) => [storyName]
}

@shilman
Copy link
Member

shilman commented Jun 23, 2017

@igor-dv how about just resolveStoryHierarchy ... or something a little shorter! 😄

@shilman shilman added this to the v3.2.0 milestone Jun 23, 2017
@ndelangen
Copy link
Member

@igor-dv I'm loving this! This is amazing!

@shilman shilman changed the base branch from release/3.2 to 151-story-hierarchy June 25, 2017 09:13
@shilman shilman merged commit f57a602 into storybookjs:151-story-hierarchy Jun 25, 2017
@shilman
Copy link
Member

shilman commented Jun 25, 2017

NOTE: merged this into 151-story-hierarchy branch so @igor-dv can continue working in the storybook repo. 👍

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.

None yet

6 participants