-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Vue3: Fix CSF2 support with decorators #20995
Conversation
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.
Hi @chakAs3 !!! Thank you for fixing this 🙏
I don't know Vue well enough to comment on the core changes. However, I have a few questions about the tests.
// fetch the story with the updated context (with reactive args) | ||
const element: StoryFnVueReturnType = storyFn(storyContext); | ||
|
||
if (!element) { |
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.
Why are we removing error handling 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.
yes, i removed it because it will never happen as we don't need to define the template any more, especially in CSF3, but I don't know if CSF2 requires this... Please guide me on this, anyway will put it back for now
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.
so if can see this will just work even if the story template has no component
export const WithRenderTemplate: Story = {
args: {
label: 'Button',
size: 'small',
},
render(args: any){
return ({
components: { Button },
setup() {
return { args };
},
template: '<b> No Button Component <pre>{{ args }}</pre></b>' ,
})
},
};
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.
@shilman I have done more tests and even with CSF2 no error rises, However, Vue handles the case when there is no template defined,it shows a Warning message on the console.
let me know if still want me to keep it.
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.
Also, this snapshot test looks wrong:
https://www.chromatic.com/test?appId=630513346a8e284ae244d415&id=63e68ea075d92124fe23bb78
Looking at the play function, it seems like the story rendering in chromatic is inconsistent with the play function assertions
Yes should fail sorry i forgot to pass to reactiveArgs variable instead of context.args it should be fine now |
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.
Looks great! Any chance you can add a test case for CSF2 before we merge?
Closes #20903
that was one issue reported by user, CSF2 does not work in V7 + one issue I spotted in rendering decorated Stories without a defined render function
What I did
I fixed the rendering for stories with decorators both for CSF2 and CSF3 + some optimisation and code refactoring
How to test
yarn task --task sandbox --start-from auto --template vue3-vite/default-ts
Checklist
MIGRATION.MD
Maintainers
make sure to add the
ci:merged
orci:daily
GH label to it.["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]