Feature/multiple hierarchies #2452

havelaer · 2017-12-09T11:44:52Z

What I did

Extended the hierarchy functionality to enable multiple hierarchies next to the main (nameless hierarchy).
I added an extra rootSeparator (not sure about the name though):

setOptions({
  // ... 
  hierarchySeparator: /\./,
  rootSeparator: /\|/, // default is false
});

UPDATED: rootSeparator is renamed to hierarchyRootSeparator with default value null

Extending the API as such:

storiesOf('Addons|Addon Info.React Docgen', module)
  .add(          ^
  ...

Works backwards compatible with or without setting the hierarchySeparator.

Documentation is updated in addons/options/README.md.

Allowing multiple hierarchies, see issue #2451 for screenshot.

How to test

I added the rootSeparator to the kitchen sink example and added the "|" separator to the addon stories.

See cra-kitchen-sink for an example.

The tests are fixed and extended. Though 1 test is failing, but that it is failing on master as well:

FAIL  examples/cra-kitchen-sink/src/storyshots.test.js
  ● Test suite failed to run

    TypeError: (0 , _react3.storiesOf)(...).add(...).add(...).add(...).addWithInfo is not a function

Hypnosphi · 2017-12-09T12:05:38Z

examples/cra-kitchen-sink/src/stories/addon-a11y.stories.js

@@ -6,7 +6,7 @@ import BaseButton from '../components/BaseButton';

 const text = 'Testing the a11y addon';

-storiesOf('Addon a11y', module)
+storiesOf('Addons|Addon a11y', module)


Looks like now we can drop the "Addon" word everywhere:
'Addons|A11y'

Hypnosphi · 2017-12-09T12:07:48Z

1 test is failing, but that it is failing on master as well

That's weird. Did you run yarn bootstrap --core?

Hypnosphi · 2017-12-09T12:13:18Z

Can you please resolve the conflicts? They are mostly caused by renaming "Left panel" to "Stories panel"

codecov · 2017-12-09T12:35:02Z

Codecov Report

Merging #2452 into master will increase coverage by <.01%.
The diff coverage is 48.71%.

@@            Coverage Diff             @@
##           master    #2452      +/-   ##
==========================================
+ Coverage   34.33%   34.34%   +<.01%     
==========================================
  Files         388      389       +1     
  Lines        8682     8750      +68     
  Branches      901      916      +15     
==========================================
+ Hits         2981     3005      +24     
- Misses       5097     5131      +34     
- Partials      604      614      +10

Impacted Files	Coverage Δ
lib/ui/src/modules/api/index.js	`0% <ø> (ø)`	⬆️
lib/ui/src/modules/ui/libs/filters.js	`48.64% <0%> (+1.28%)`	⬆️
addons/options/src/preview/index.js	`0% <0%> (ø)`	⬆️
.../ui/components/stories_panel/stories_tree/index.js	`38.73% <0%> (-3.26%)`	⬇️
...onents/stories_panel/stories_tree/index.stories.js	`94.44% <100%> (ø)`	⬆️
...dules/ui/components/stories_panel/index.stories.js	`90.9% <100%> (ø)`	⬆️
...i/src/modules/ui/components/stories_panel/index.js	`20.96% <25%> (+2.44%)`	⬆️
...mponents/stories_panel/stories_tree/tree_header.js	`18.75% <27.27%> (ø)`
lib/ui/src/modules/ui/containers/stories_panel.js	`27.02% <41.66%> (+1.31%)`	⬆️
lib/ui/src/modules/ui/libs/hierarchy.js	`49.51% <70%> (-1.24%)`	⬇️
... and 64 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 51108ef...aaff55f. Read the comment docs.

havelaer · 2017-12-09T12:36:02Z

@Hypnosphi Thanks for reviewing!
The branch was based on master. I've rebased the branch on latest release/3.3 and fixed the conflicts.
yarn bootstrap reset everything and all tests are passing!
I've also omitted 'Addon' in the addon story names.

Hypnosphi · 2017-12-09T13:31:52Z

lib/ui/src/modules/ui/components/stories_panel/sub_header.js

+const headingStyle = {
+  ...baseFonts,
+  textTransform: 'uppercase',
+  // letterSpacing: '1.5px',


Looks like it's worth uncommenting. Uppercase text is more readable when it has some letter-spacing

Changed it to letterSpacing: '1.2px'

Hypnosphi · 2017-12-09T13:34:22Z

Please run yarn test --core --update to update test snapshots

Hypnosphi · 2017-12-09T13:36:42Z

Live example for reference

Looks like some of the stories still have wrong kinds:

Hypnosphi · 2017-12-09T13:38:27Z

Root title is hidden when using search. Is it on purpose?

igor-dv

Very like this ability. Would like to use for separating core component in my project.
I didn't check the tests yet
I think overloading the storyKind to have more special syntax is a bit wrong direction, but currently, we have nothing to do with it. (maybe create some kind of URI syntax for story) @ndelangen , you have plans for refactoring, WDYT?

igor-dv · 2017-12-09T12:47:44Z

addons/options/src/preview/index.js

+  // override the hierarchySeparator or rootSeparator if the prop is missing
+  const options = {
+    ...newOptions,
+    ...(hasOwnProp(newOptions, 'hierarchySeparator')


I think worth extracting this to a method - for readability. withProp() or something.

const option = { ...newOptions, ...withProp(newOptions, 'hierarchySeparator'), ...withProp(newOptions, 'rootSeparator'), }

Done! Called it withRegexProp()

igor-dv · 2017-12-09T12:50:03Z

addons/options/README.md

+   *   /\|/ - split by `|`
+   * @type {Regex}
+   */
+  rootSeparator: null,


Maybe better call it hierarchyRootSeparator for naming consolidation

Could the PR description be updated to have it in the example there? Didn't work on the first try and had to search around to discover this.

Found my way here through the 3.4.0-alpha0 changelog entry

igor-dv · 2017-12-09T13:23:19Z

lib/ui/src/modules/ui/containers/stories_panel.js

-  const selectedHierarchy = resolveStoryHierarchy(selectedKind, hierarchySeparator);
+  const storiesHierarchies = createHierarchies(filteredStories);
+
+  const [, storyName] = resolveStoryRoots(selectedKind, rootSeparator);


I don't understand why exporting the resolveStoryRoots ? You can hide it under the resolveStoryHierarchy method.

resolveStoryRoots is exported for 2 reasons:

It's also needed in the stories_panel container.

It's has separate unit tests.

Btw I renamed resolveStoryRoots to resolveStoryHierarchyRoots.

igor-dv · 2017-12-09T13:29:29Z

lib/ui/src/modules/ui/libs/hierarchy.js

@@ -46,24 +46,47 @@ function fillHierarchy(namespaces, hierarchy, story) {
  fillHierarchy(namespaces.slice(1), childHierarchy, story);
 }

-export function createHierarchy(stories) {
-  const hierarchyRoot = {
+export function createHierarchyRoot(name = '') {


Looks like you export this function only for tests, IMO it's not a good thing.

It's not the main js file, so I think it's Ok to have test-only exports for more granular unit tests

igor-dv · 2017-12-09T13:34:29Z

lib/ui/src/modules/ui/libs/hierarchy.js

+
+export function resolveStoryRoots(storyName = '', rootSeparator) {
+  if (!rootSeparator) {
+    return ['', storyName];


I think in this method better return an object, instead of relying on indices in the array.

Agreed. Changed it, so it returns an object.

Btw I renamed resolveStoryRoots to resolveStoryHierarchyRoots.

igor-dv · 2017-12-09T13:38:57Z

lib/ui/src/modules/ui/libs/hierarchy.js


      fillHierarchy(namespaces, hierarchyRoot, { ...story, name });
    });
  }

-  return hierarchyRoot;
+  return Object.keys(rootMap).map(rootName => rootMap[rootName]);


Object.values(rootMap) ?

Polyfills take care of Object.values?

Code is changed a bit at those lines though: I had to make sure that the main hierarchy is always the first in the array, so I didn't have use for Object.values anymore.

Which triggers the next question:
What could we do with the sorting of the other hierarchies in the panel? Now it respects the order in which the stories are loaded.

igor-dv · 2017-12-09T13:45:42Z

lib/ui/src/modules/ui/components/stories_panel/sub_header.js

+);
+
+SubHeader.defaultProps = {
+  name: '',


what do you think about changing name to children ? I think it would be nicer for use. And I think it should be required.

Agreed! Initially copied stories_panel/header.js props interface. But I prefer children prop in such situations. So changed it.

igor-dv · 2017-12-09T13:49:07Z

lib/ui/src/modules/ui/components/stories_panel/index.js

-          {hierarchyContainsStories(storiesHierarchy) ? (
-            <Stories {...pick(this.props, storyProps)} />
-          ) : null}
+          {storiesHierarchies.map(


worth extracting it to function already.

extracted the map it to separate render method.
extracted the sub header to stories_tree/ as tree_header

havelaer · 2017-12-09T16:30:17Z

Looks like some of the stories still have wrong kinds:

Fixed!

Root title is hidden when using search. Is it on purpose?

Nice catch! Fixed!

Hypnosphi

LGTM. Let's merge it right after the upcoming 3.3.0 release

igor-dv

Very nice tests! + tested locally.

igor-dv · 2017-12-10T06:31:44Z

lib/ui/src/modules/ui/libs/hierarchy.js

+  const rootNames = Object.keys(rootMap);
+  const hierarchies = rootNames.map(rootName => rootMap[rootName]);
+
+  // make sure the main root is the first


Please extract this block to function - moveRootToFront / ensureRootIsFirst or something

Extracted to ensureMainRootIsFirst and used Object.values instead of Object.keys(...).map(...)

igor-dv · 2017-12-10T06:35:34Z

lib/ui/src/modules/ui/libs/hierarchy.js

@@ -46,24 +46,47 @@ function fillHierarchy(namespaces, hierarchy, story) {
  fillHierarchy(namespaces.slice(1), childHierarchy, story);
 }

-export function createHierarchy(stories) {
-  const hierarchyRoot = {
+export function createHierarchyRoot(name = '') {


ndelangen · 2017-12-12T14:27:20Z

Good to go if you ask me!

danielduan · 2017-12-12T17:16:32Z

I think we're aiming to add this for 3.4?

I added the do not merge label because of the upcoming changes to our workflow. Feel free to merge to master after we get 3.3 out

Hypnosphi · 2017-12-25T13:56:21Z

We have moved some stories to examples/storybook-official. @havelaer Would you mind resolving the conflicts?

Hypnosphi · 2017-12-27T11:55:58Z

@shilman after we merge this one, we won't be able to release 3.3 from master anymore. There's probably a sense in merging master into release/3.3 before that

havelaer · 2017-12-27T11:58:58Z

I've rebased the branch on master, keeping the history nice & clean. Fixed tests, added separator config to examples/official-storybook and updated core and cli snapshots.

ndelangen · 2017-12-27T12:14:59Z

@havelaer That's fantastic work!

igor-dv · 2018-01-03T22:51:03Z

@havelaer maybe you can resolve the conflicts and merge?

# Conflicts: # examples/official-storybook/config.js # lib/cli/test/snapshots/angular-cli/package.json # lib/cli/test/snapshots/meteor/package.json # lib/cli/test/snapshots/react/package.json # lib/cli/test/snapshots/react_native/package.json # lib/cli/test/snapshots/react_native_scripts/package.json # lib/cli/test/snapshots/react_project/package.json # lib/cli/test/snapshots/react_scripts/package.json # lib/cli/test/snapshots/sfc_vue/package.json # lib/cli/test/snapshots/update_package_organisations/package.json # lib/cli/test/snapshots/vue/package.json # lib/cli/test/snapshots/webpack_react/package.json

ndelangen · 2018-01-04T12:23:56Z

I resolved merge conflicts

igor-dv · 2018-01-04T13:13:10Z

👏

Hypnosphi reviewed Dec 9, 2017

View reviewed changes

Hypnosphi self-assigned this Dec 9, 2017

havelaer force-pushed the feature/multiple-hierarchies branch from 8c5d62f to e32dfe7 Compare December 9, 2017 12:35

Hypnosphi added feature request ui labels Dec 9, 2017

havelaer force-pushed the feature/multiple-hierarchies branch from e32dfe7 to d38a031 Compare December 9, 2017 13:17

Hypnosphi requested a review from igor-dv December 9, 2017 13:28

Hypnosphi requested changes Dec 9, 2017

View reviewed changes

igor-dv requested changes Dec 9, 2017

View reviewed changes

havelaer force-pushed the feature/multiple-hierarchies branch from 0a7ce0a to c82e426 Compare December 9, 2017 16:27

Hypnosphi added this to the v3.4.0 milestone Dec 9, 2017

Hypnosphi approved these changes Dec 9, 2017

View reviewed changes

igor-dv approved these changes Dec 10, 2017

View reviewed changes

havelaer force-pushed the feature/multiple-hierarchies branch from 39c0454 to df1694e Compare December 10, 2017 10:38

ndelangen approved these changes Dec 12, 2017

View reviewed changes

danielduan added the ci: do not merge label Dec 12, 2017

Hypnosphi changed the base branch from release/3.3 to master December 23, 2017 23:57

havelaer added 2 commits December 27, 2017 10:41

enable multiple hierarchies

e0084b5

extend kitchen sink example with multiple hierarchies

dbaf4e5

havelaer added 11 commits December 27, 2017 10:41

added missing addons to addon hierarchy

fba5b3b

restored letterspacing sub header

ae6d313

updated snapshots

1276f25

change rootSeparator to hierarchyRootSeparator

bd025b7

sub header text as required children

a13ac22

refactor stories panel

fccff4f

refactor and fix hierarchy helpers

283c7ce

improved tests touched files

f6775ce

extracted main root is first of hierarchies

a12726f

post rebase origin/master fixes

b3a6b01

update snapshots

05a0699

havelaer force-pushed the feature/multiple-hierarchies branch from 630a9c9 to 05a0699 Compare December 27, 2017 09:56

updated cli snapshots

b38efde

ndelangen approved these changes Dec 27, 2017

View reviewed changes

Hypnosphi added ci: do not merge and removed ci: do not merge labels Dec 27, 2017

Hypnosphi added the ready label Dec 27, 2017

ndelangen approved these changes Jan 3, 2018

View reviewed changes

Hypnosphi removed the ci: do not merge label Jan 3, 2018

Merge branch 'master' into feature/multiple-hierarchies

aaff55f

igor-dv merged commit ece503a into storybookjs:master Jan 4, 2018

dependencies bot mentioned this pull request May 14, 2018

Update 80 dependencies from npm #3584

Merged

jstnjns mentioned this pull request Jun 29, 2018

How should we support static html in storybook? yankaindustries/mc-components#16

Closed

Feature/multiple hierarchies #2452

Feature/multiple hierarchies #2452

Conversation

havelaer commented Dec 9, 2017 • edited

What I did

How to test

Choose a reason for hiding this comment

Hypnosphi commented Dec 9, 2017

Hypnosphi commented Dec 9, 2017

codecov bot commented Dec 9, 2017 • edited

Codecov Report

havelaer commented Dec 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hypnosphi commented Dec 9, 2017

Hypnosphi commented Dec 9, 2017

Hypnosphi commented Dec 9, 2017

igor-dv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

havelaer commented Dec 9, 2017

Hypnosphi left a comment

Choose a reason for hiding this comment

igor-dv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndelangen commented Dec 12, 2017

danielduan commented Dec 12, 2017

Hypnosphi commented Dec 25, 2017 • edited

Hypnosphi commented Dec 27, 2017 • edited

havelaer commented Dec 27, 2017

ndelangen commented Dec 27, 2017

igor-dv commented Jan 3, 2018

ndelangen commented Jan 4, 2018

igor-dv commented Jan 4, 2018

havelaer commented Dec 9, 2017 •

edited

codecov bot commented Dec 9, 2017 •

edited

Hypnosphi commented Dec 25, 2017 •

edited

Hypnosphi commented Dec 27, 2017 •

edited