-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Svelte support #3770
Svelte support #3770
Conversation
Oh, one more framework support, that's awesome! |
0690b24
to
f64ec68
Compare
Codecov Report
@@ Coverage Diff @@
## master #3770 +/- ##
==========================================
- Coverage 39.36% 38.87% -0.49%
==========================================
Files 430 442 +12
Lines 5475 5546 +71
Branches 740 744 +4
==========================================
+ Hits 2155 2156 +1
- Misses 2935 3001 +66
- Partials 385 389 +4
Continue to review full report at Codecov.
|
addons/backgrounds/svelte.js
Outdated
@@ -0,0 +1 @@ | |||
module.exports = require('./dist/deprecated'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is deprecated, so you don't need to add a new one. You should just import from the @storybook/addon-backgrounds
</div> | ||
|
||
<style> | ||
.style { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could styles be imported from the ./styles.js
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can, but it will make the svelte template "worse". The style attribute doesn't work the same way as in react or vue, so I can't just pass the object in there.
I'll have a go, and see what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm adding a small helper function to convert it to plain CSS strings before applying it inline in the component. Not very pretty but solves the problem. It doesn't make the component 'worse', so I guess there's that.
addons/centered/src/index.js
Outdated
@@ -2,5 +2,15 @@ import { window } from 'global'; | |||
import ReactCentered from './react'; | |||
import VueCentered from './vue'; | |||
|
|||
const Centered = window.STORYBOOK_ENV === 'vue' ? VueCentered : ReactCentered; | |||
function getCentered(env) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the purpose of this refactoring?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, this is from when I didn't understand that the other frameworks are importing the centered addon directly.
I have used Vue as the main starting point since it works in the closest way. So I had svelte in here too, but didn't like it.
I'll checkout this file again from master.
addons/knobs/svelte.js
Outdated
@@ -0,0 +1 @@ | |||
module.exports = require('./dist/deprecated'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, knobs are framework agnostic.
addDecorator, | ||
addParameters, | ||
configure, | ||
getStorybook, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can also export here the forceReRender
.
@@ -0,0 +1,12 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't the root .babelrc
match your needs?
"dev": "cross-env NODE_ENV=development webpack-dev-server --open --hot", | ||
"storybook": "start-storybook -p 9009 -s public" | ||
}, | ||
"dependencies": {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should probably add svelte
deps here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Svelte components are compiled, so all Svelte components are vanilla js functions. So, no runtime deps.
work correctly both with client-side routing and a non-root public URL. | ||
Learn how to configure a non-root public URL by running `npm run build`. | ||
--> | ||
<title>React App</title> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
React ?
ADDONS_SUPPORT.md
Outdated
|[links](addons/links) |+|+|+|+|+|+|+| | | | ||
|[notes](addons/notes) |+| |+|+|+|+|+| | | | ||
|[options](addons/options) |+|+|+|+|+|+|+| | | | ||
|[storyshots](addons/storyshots) |+|+|+|+| | |+| | | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please mark all the supported addons
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm working my way through them, marking them off and committing as I go. I think I've marked off everything I've tested so far.
@@ -0,0 +1,32 @@ | |||
import global from 'global'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any committed snapshots, does this addon really work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not yet, I had to stop and sleep yesterday, got a bit late 😸
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should now be generating snapshots.
app/svelte/package.json
Outdated
"name": "@storybook/svelte", | ||
"version": "4.0.0-alpha.9", | ||
"description": "Storybook for Svelte: Develop Svelte Component in isolation with Hot Reloading.", | ||
"homepage": "https://github.com/storybooks/storybook/tree/master/apps/vue", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
vue?
@@ -1,11 +1,13 @@ | |||
import { storiesOf } from '@storybook/vue'; | |||
import Centered from '@storybook/addon-centered'; | |||
|
|||
import MyButton from './Button.vue'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not relevant to this review.
From what I can see with the CodeFactor issues, these exist in all the other framework examples. If this is acceptable, is there anything else I need to do before this is merged into master? |
Ah, the chromatic test. |
Hey, to fix chromatic, you just need to change the file |
# Conflicts: # package.json
I created the symlink but this did not resolve the chromatic issue. Also a unittest is failing:
|
It is because a wrong alpha versions after the merge. Everything should be 16 |
Thanks for that @igor-dv, I fixed it. |
Ok, I think we've tried everything to solve chromatic. I've even tried to exorcise my laptop... |
Just as a note -- I could get the Acceptance story running properly locally when i removed the file at I suspect the solution is to remove the file that currently exists, commit, recreate the symlink, then commit again. I can do it if you like -- not sure where to commit to? |
This file was my attempt to create a symlink on windows 😓 |
Sorry I didn't realise I could push to the fork. Did that, hopefully fixed now. |
Ugh, Ok hopefully did it properly this time. |
Yes! Thank you so much guys. Really appreciate you not only accepting it, but helping out to push it out too. |
Issue: #2984
What I did
How to test
In root folder
yarn test
Select
core
tests.Is this testable with Jest or Chromatic screenshots? Yes, both
Does this need a new example in the kitchen sink apps? Yes
Does this need an update to the documentation? Yes