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

Story Hierarchy improved UI #1356

Merged
merged 21 commits into from
Jun 29, 2017
Merged

Story Hierarchy improved UI #1356

merged 21 commits into from
Jun 29, 2017

Conversation

shilman
Copy link
Member

@shilman shilman commented Jun 25, 2017

Issue: #151

What I did

Continuation of #1329, but in the main storybook repo

How to test

Run cra-kitchen-sink (see #1329)

@codecov
Copy link

codecov bot commented Jun 25, 2017

Codecov Report

Merging #1356 into release/3.2 will increase coverage by 0.94%.
The diff coverage is 71.13%.

Impacted file tree graph

@@               Coverage Diff               @@
##           release/3.2    #1356      +/-   ##
===============================================
+ Coverage        14.35%   15.29%   +0.94%     
===============================================
  Files              201      202       +1     
  Lines             4613     4706      +93     
  Branches           602      508      -94     
===============================================
+ Hits               662      720      +58     
- Misses            3447     3563     +116     
+ Partials           504      423      -81
Impacted Files Coverage Δ
lib/ui/src/modules/api/index.js 0% <ø> (ø) ⬆️
lib/ui/example/client/provider.js 0% <0%> (ø) ⬆️
...b/ui/src/modules/ui/components/left_panel/index.js 100% <100%> (ø) ⬆️
...ui/src/modules/ui/components/left_panel/stories.js 100% <100%> (ø) ⬆️
lib/ui/src/modules/ui/libs/hierarchy.js 49.2% <58.49%> (ø)
lib/ui/src/modules/ui/containers/left_panel.js 25.71% <66.66%> (+5.71%) ⬆️
app/react/src/server/babel_config.js 44.82% <0%> (ø) ⬆️
lib/ui/src/modules/ui/configs/init_panels.js 25% <0%> (ø) ⬆️
addons/info/src/components/Props.js 0% <0%> (ø) ⬆️
... and 29 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 74fde7b...8262b7b. Read the comment docs.

@@ -8,6 +8,7 @@ export default {
name: 'STORYBOOK',
url: 'https://github.com/storybooks/storybook',
sortStoriesByKind: false,
resolveStoryHierarchy: storyName => [storyName],
Copy link
Member

Choose a reason for hiding this comment

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

I guess we'll lose a serialized function after passing through the channel.
We can consider options:

  • pass only JSON data here
  • provide this API on the manager side (it'll be available in addons.js)

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 I'll just change it to be symbol/regex.
hierarchySeparator: '/' or hierarchySeparator: '.'

@igor-dv
Copy link
Member

igor-dv commented Jun 26, 2017

Ok. (Beside the thing I need to fix with what @usulpro pointed on)

Here is the experimental use of react-treebeard (because of this)

storybook_hierarchy_react-treebeard

There are warnings because of this, but it looks very promising.

@@ -177,16 +177,16 @@ storiesOf('component.Button')

// Atomic

storiesOf('Atoms.Molecules.Cells.simple', module)
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.

awesome ❤️

@igor-dv
Copy link
Member

igor-dv commented Jun 29, 2017

Some updates for the new version

react-with-css-with-styleguide-2

I've added Ionicons from the react-icons.

@usulpro
Copy link
Member

usulpro commented Jun 29, 2017

Looks great! If we got this package in dependencies I can switch to it in #981 as well

@igor-dv
Copy link
Member

igor-dv commented Jun 29, 2017

I thought it would be great to have it integrated to theming api too

@@ -15,8 +15,7 @@ module.exports = latestVersion('@storybook/react-native').then(version => {
packageJson.devDependencies['@storybook/react-native'] = `^${version}`;

if (!packageJson.dependencies['react-dom'] && !packageJson.devDependencies['react-dom']) {
const reactVersion = packageJson.dependencies.react;
packageJson.devDependencies['react-dom'] = reactVersion;
packageJson.devDependencies['react-dom'] = '^15.5.4';
Copy link
Member Author

Choose a reason for hiding this comment

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

hmm?

Copy link
Member

@igor-dv igor-dv Jun 29, 2017

Choose a reason for hiding this comment

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

idk what is this ¯\(ツ)/¯ ? Maybe it was somehow merged from the previous PR ?

@@ -31,8 +31,7 @@ module.exports = latestVersion('@storybook/react-native').then(version => {
packageJson.devDependencies['@storybook/react-native'] = `^${version}`;

if (!packageJson.dependencies['react-dom'] && !packageJson.devDependencies['react-dom']) {
const reactVersion = packageJson.dependencies.react;
packageJson.devDependencies['react-dom'] = reactVersion;
packageJson.devDependencies['react-dom'] = '^15.5.4';
Copy link
Member Author

Choose a reason for hiding this comment

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

hmm?


// Atomic

storiesOf('Atoms¯\\_(ツ)_/¯Molecules.Cells/simple', module)
Copy link
Member Author

Choose a reason for hiding this comment

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

should be cells/molecules/atoms? current order doesn't make sense to me?

Copy link
Member

Choose a reason for hiding this comment

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

I'll fix it.

package.json Outdated
@@ -58,7 +58,7 @@
"jest": "^20.0.4",
"jest-enzyme": "^3.2.0",
"lerna": "2.0.0-rc.5",
"lint-staged": "^4.0.0",
"lint-staged": "^3.5.1",
Copy link
Member Author

Choose a reason for hiding this comment

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

downgrade?

@igor-dv
Copy link
Member

igor-dv commented Jun 29, 2017

Yeah, it's green now =)

@usulpro
Copy link
Member

usulpro commented Jun 29, 2017

Is it ready to check out?

'storiesHierarchy',
'selectedKind',
'selectedHierarchy',
'selectedStory',
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need to have selectedStory and selectedKind in propTypes?

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 linter cries that they are not in use in here. these props just passed further to stories.js where they are actually defined in prop types.

@igor-dv
Copy link
Member

igor-dv commented Jun 29, 2017

I think it's ready, but non of the reviewers acepted the PR ...

@shilman
Copy link
Member Author

shilman commented Jun 29, 2017

@igor-dv @usulpro LGTM! Github is not letting me approve the PR because I was the one who opened it 😖

@shilman
Copy link
Member Author

shilman commented Jun 29, 2017

@igor-dv I guess we will wait to merge the treebeard improvements until after the new version is ready on NPM?

@igor-dv
Copy link
Member

igor-dv commented Jun 29, 2017

@shilman , yes, I think so. I'll merge this one and then will open a new PR for the improvements.

@shilman shilman added cleanup Minor cleanup style change that won't show up in release changelog and removed feature request labels Jul 27, 2017
@shilman shilman changed the title Story Hierarchy (continued) Story Hierarchy improved UI Jul 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Minor cleanup style change that won't show up in release changelog ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants