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

Vue: Add CSF3 default render function #17279

Merged
merged 8 commits into from Jan 24, 2022
Merged

Conversation

yannbf
Copy link
Member

@yannbf yannbf commented Jan 18, 2022

Issue: #15498

What I did

Added Vue default render function

How to test

1 - Run storybook in vue-kitchen-sink
2 - See Button/WithDefaultRender

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

@nx-cloud
Copy link

nx-cloud bot commented Jan 18, 2022

☁️ Nx Cloud Report

CI ran the following commands for commit f7df112. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Hi @yannbf ! I'm not a Vue expert but I don't think the default render function is correct. If you go to the new story in vue-kitchen-sink, it does not update when you update the controls.

This works, but the source snippet shows component instead of my-button, so I don't think it's a good solution. I don't know enough Vue to know the right answer here:

  return {
    props: Object.keys(argTypes),
    components: { Component },
    template: '<component v-bind="$props" />',
  };

@yannbf
Copy link
Member Author

yannbf commented Jan 21, 2022

Alright I reworked the render function resulting in the following snippets:

1 - component with correct name property

// Button.vue
export default {
  name: 'my-button' // correct, should be used as is
}

image

2 - component with invalid name

// Button.vue
export default {
  name: 'button' // clashes with HTML reserved tag, should be prepended with sb
}

image

3 - component with name in PascalCase

// Button.vue
export default {
  name: 'MyComponentName' // JSX name, should be used as is
}

image

4 - component with no name property

// Button.vue
export default {
  // with no name property, name should be extracted from file name
}

image


What I'm unsure is about scenarios 3 and 4. Should the JSX name be used? Should we always kebab-case them?

@pocka @prashantpalikhe would love to hear your perspective!

@pocka
Copy link
Contributor

pocka commented Jan 21, 2022

@yannbf

What I'm unsure is about scenarios 3 and 4. Should the JSX name be used? Should we always kebab-case them?

I feel both results are quite intuitive.

For scenario 3, we can't guess the user's preference because PascalCase name can be used for both kebab-case and PascalCase usage. Since the official style guide recommends using PascalCase instead of kebab-case, we should keep the PascalCase name, IMO.

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

@yannbf Looking great, but Storyshots is failing in CI. Looks like that global registration was there to illustrate a particular use case:

Storyshots › Custom/Method for rendering Vue › pre-registered component
    error: [Vue warn]: Unknown custom element: <my-button> - did you register the component correctly? For recursive components, make sure to provide the "name" option.

@yannbf
Copy link
Member Author

yannbf commented Jan 21, 2022

@yannbf Looking great, but Storyshots is failing in CI. Looks like that global registration was there to illustrate a particular use case:

Storyshots › Custom/Method for rendering Vue › pre-registered component
    error: [Vue warn]: Unknown custom element: <my-button> - did you register the component correctly? For recursive components, make sure to provide the "name" option.

Oops. Fixed!

@prashantpalikhe
Copy link
Contributor

Hi @yannbf

I think when providing these defaults, the best is to mimic the behavior of Vue dev tools.

Vue dev tools seem to only display component names in PascalCase, even when the name is provided in kebab-case in the component definition. E.g. name: "some-button" would still appear as SomeButton in the dev tools.

The provided name takes precedence overall.

If no name is provided, the chosen display name is the name of the import. E.g. import Btn from 'Button' would show <Btn /> in the dev tools.

In the Storybook case, users would probably do import Button from 'Button'. And since that is the reserved tag, it would lead to an error. SB could prefix the component automatically with sb- indeed. But, also let Vue error out the same way as in a Vue application. Do not use reserved tag.... I would be ok with both, I think. Need to play with that a little.

So to answer your questions:

Q3: Since Vue dev tools always prefer PascalCase, I'd go the same way.
Q4: Use the import name. Either let Vue error on reserved tag or auto prefix with sb-.

Also, my suggestion is to play more with all your use cases and see how Vue dev tools handle things. And stick to that behavior.

@shilman shilman merged commit 0300858 into next Jan 24, 2022
@shilman shilman deleted the csf3-vue-default-render-fn branch January 24, 2022 15:56
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.

None yet

4 participants