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

Improve the decorator for vue #1595

Merged
merged 15 commits into from
Aug 18, 2017

Conversation

kazupon
Copy link
Member

@kazupon kazupon commented Aug 4, 2017

Issue: #1529

What I did

Improve the decorator for Vue app storybook

How to test

  • npm i
  • npm run bootstrap
  • cd app/vue
  • npm link
  • cd ../../examples/vue-kitchen-sink
  • npm link
  • npm run storybook

@ndelangen
Copy link
Member

Hey @kazupon you should be able to make branches on this repo and push to them, right?
I find reviewing forks a bit more involved from reviewing branches. FYI.

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.

Thanks for this!

Tested locally ✅

@ndelangen
Copy link
Member

ndelangen commented Aug 5, 2017

We will possibly revisit the decorator concept with 4.0.0, but for now I think this will help starting vue-users.

#1209

@ndelangen
Copy link
Member

Can you fix the unit tests? it's probably just a matter of updating the snapshots

@ndelangen ndelangen requested a review from a team August 5, 2017 08:59
@kazupon
Copy link
Member Author

kazupon commented Aug 5, 2017

fixed unit tests.

@ndelangen
Should I support centered official decorator for Vue?
https://github.com/storybooks/storybook/tree/master/addons/centered

@codecov
Copy link

codecov bot commented Aug 5, 2017

Codecov Report

Merging #1595 into master will increase coverage by 0.15%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1595      +/-   ##
==========================================
+ Coverage   20.68%   20.84%   +0.15%     
==========================================
  Files         249      251       +2     
  Lines        5588     5575      -13     
  Branches      669      685      +16     
==========================================
+ Hits         1156     1162       +6     
+ Misses       3914     3892      -22     
- Partials      518      521       +3
Impacted Files Coverage Δ
addons/centered/src/vue.js 0% <0%> (ø)
addons/centered/src/react.js 0% <0%> (ø)
addons/centered/src/index.js 0% <0%> (ø) ⬆️
app/vue/src/client/preview/client_api.js 87.23% <87.5%> (-0.27%) ⬇️
addons/knobs/src/react/WrapStory.js 12% <0%> (ø) ⬆️
addons/knobs/src/components/types/Boolean.js 11.62% <0%> (ø) ⬆️
addons/knobs/src/components/types/Color.js 8% <0%> (ø) ⬆️
addons/knobs/src/components/types/Number.js 8.06% <0%> (ø) ⬆️
addons/knobs/src/KnobManager.js 32% <0%> (ø) ⬆️
addons/knobs/src/KnobStore.js 6.81% <0%> (ø) ⬆️
... and 30 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 f6ac4a8...f1bf341. Read the comment docs.

@ndelangen
Copy link
Member

Well in the end, I want decorators to be framework independent.

Do you see any way of making this happen for addon-centered?

Even if not, I think Vue users will want the same thing, So I'd accept a PR for that.

@kazupon
Copy link
Member Author

kazupon commented Aug 7, 2017

okay,
I seem the it can be support addon-centered for both react and vue.
I'll try to it. 💪

@kazupon
Copy link
Member Author

kazupon commented Aug 11, 2017

done!

@ndelangen
Copy link
Member

I fixed a merge-conflict in the package.json dependencies.

@ndelangen ndelangen merged commit afeebe0 into storybookjs:master Aug 18, 2017
@kazupon kazupon deleted the improve/decorator-for-vue branch August 18, 2017 14:05
@kazupon
Copy link
Member Author

kazupon commented Aug 18, 2017

Thanks!

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.

2 participants