feat: add Vue support to @addons/background #3488
Conversation
wuweiweiwu
requested review from
alexandrebodin,
Hypnosphi,
igor-dv and
ndelangen
as code owners
addons/background/src/index.js
Outdated
|
|
||
| import addons from '@storybook/addons'; | ||
| const Background = window.STORYBOOK_ENV === 'vue' ? VueBackground : ReactBackground; |
There was a problem hiding this comment.
The problem with this approach is that now you bring Vue to react storybook bundle when it's not actually needed. In knobs, we decided to have separate entry points for each framework:
import { withKnobs } from '@storybook/addon-knobs/react'
see #1832 for reference
There was a problem hiding this comment.
But actually, there's a chance we come up with a generic framework-agnostic solution for addons like this, which only add subscriptions but don't render anything inside preview iframe:
See #3475 (comment)
In this case, there will be no need in having different decorators in different frameworks
There was a problem hiding this comment.
I see. I will follow this up with a PR to update @addons/centered since this is what it is currently doing.
generic framework agnostic addons would awesome!
There was a problem hiding this comment.
Should I wait for subscriptionStore to be merged? or should I just do framework specific exports for now?
There was a problem hiding this comment.
I think you can add a temporary vue entry point without renaming index to react
Codecov Report
@@ Coverage Diff @@
## master #3488 +/- ##
==========================================
+ Coverage 37.42% 37.43% +<.01%
==========================================
Files 455 457 +2
Lines 10296 10304 +8
Branches 934 912 -22
==========================================
+ Hits 3853 3857 +4
- Misses 5860 5878 +18
+ Partials 583 569 -14
Continue to review full report at Codecov.
|
|
@Hypnosphi fixed! :) |
| @@ -30,6 +30,9 @@ | |||
| "prop-types": "^15.6.1", | |||
| "react-lifecycles-compat": "^3.0.2" | |||
| }, | |||
| "devDependencies": { | |||
| "vue": "^2.5.16" | |||
| }, | |||
| "peerDependencies": { | |||
| "react": "*" | |||
There was a problem hiding this comment.
I think we should remove this peerDependency now (and suppress corresponding ESLint warnings)
There was a problem hiding this comment.
Do you mean the react or the vue dependency? or should I just remove both?
There was a problem hiding this comment.
or should I move the react dep into devDependencies
There was a problem hiding this comment.
I mean react. In devDependencies, you can have anything that's used in tests.
There was a problem hiding this comment.
Sounds good! Thanks for clarifying
|
I can't find these changes in v3.4.6. Is this not released yet? |
|
@dietergeerts, these changes were released on the alpha version, specifically at |
Issue:
fixes #2845
What I did
added a new decorator export for
@addons/background/vueHow to test
yarn test --coreIs this testable with Jest or Chromatic screenshots? y
Does this need a new example in the kitchen sink apps? y
Does this need an update to the documentation? y
If your answer is yes to any of these, please make sure to include it in your PR.
For maintainers only: Please tag your pull request with at least one of the following:
["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]