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

fix: improve scopedSlots option #808

Merged
merged 3 commits into from
Jul 6, 2018
Merged

fix: improve scopedSlots option #808

merged 3 commits into from
Jul 6, 2018

Conversation

38elements
Copy link
Contributor

@38elements 38elements commented Jul 5, 2018

This resolves #706.

Copy link
Member

@eddyerburgh eddyerburgh left a comment

Choose a reason for hiding this comment

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

Looks great! One small change. Can you rename the function to createScopedSlots, to be in line with createComponentStubs, createSlotVNodes, etc?

@38elements
Copy link
Contributor Author

Thank you for reviewing.
I renamed getScopedSlots to createScopedSlots.

@eddyerburgh
Copy link
Member

Thanks for making the changes.

Can you also add a test case for the example in the issue:

scopedSlots: {
  foo: (createElement, props) => { return createElement('div', props); },
},

@38elements
Copy link
Contributor Author

38elements commented Jul 6, 2018

The case does not work.
I think the feature is not necessary.
I think it is not necessary to use createElement() to make scopedSlots.
slots mounting option is not support it.
Would you please tell me why this feature is necessary?

@38elements
Copy link
Contributor Author

scopedSlotFn() receives only props.

nodes = scopedSlotFn(props) || fallback

https://github.com/vuejs/vue/blob/5a9da95b8a865416f082952a48416ffc091e4078/src/core/instance/render-helpers/render-slot.js#L27

If you think the feature is necessary, I will add an API as below.

scopedSlots: {
  default: (createElement) => (props) => { return createElement('div', props); }
}

@eddyerburgh
Copy link
Member

eddyerburgh commented Jul 6, 2018

Ok, I agree that the API does not look good. I'll merge the branch as it is 👍

@eddyerburgh eddyerburgh merged commit b946997 into vuejs:dev Jul 6, 2018
@38elements
Copy link
Contributor Author

Thanks.

@38elements
Copy link
Contributor Author

38elements commented Jul 6, 2018

#657 is not closed.
I want to confirm.
Do you think this API is necessary?

scopedSlots: {
  default: (createElement) => (props) => { return createElement('div', props); }
}

@38elements 38elements deleted the render branch July 6, 2018 13:59
@eddyerburgh
Copy link
Member

eddyerburgh commented Jul 6, 2018

Not currently, but it's a feature request so we should keep it open until we've decided definitively.

I also haven't had enough time to fully assess the scopedSlots and slots API

@38elements
Copy link
Contributor Author

38elements commented Jul 6, 2018

Thank you for reply.
I think that it is better to remove intend to impliment label for #657.

@kayandra
Copy link

kayandra commented Jul 6, 2018

@38elements thank you for this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tests for default scopedSlots not mounting
3 participants