-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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($theme-default): Fix slots syntax #1964
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,
I think we should fix it with the new syntax. What do you think?
This is my first review ever, sorry if I'm doing anything wrong.
Best Regards.
before: info => `<template #${info}>`, | ||
before: info => `<template slot="${info}">`, |
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 think this is still working fine since this is a <template>
<slot | ||
name="sidebar-top" | ||
#top | ||
slot="top" | ||
/> |
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.
The new v-slot
syntax can only be added to a <template>
, so the code below should fix it
<template #top>
<slot name="sidebar-top"/>
</template>
<CarbonAds #sidebar-top/> | ||
<BuySellAds #page-bottom/> | ||
<CarbonAds slot="sidebar-top"/> | ||
<BuySellAds slot="page-bottom"/> |
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.
We can only use the new v-slot
on the component when name is default
<template #sidebar-top>
<CarbonAds/>
</template>
Hi @ludanxer, thanks for doing a review 🙏 |
Well, I can't stop thinking it's a But the old one won't be supported anymore in vue3.0. I tied to extend |
Hi @kefranabg , I just noticed that PR #1951 already tried to fix this with the new syntax. |
Just closed this in favor of #1951 . |
Summary
Fix #1963
What kind of change does this PR introduce? (check at least one)
If changing the UI of default theme, please provide the before/after screenshot:
Does this PR introduce a breaking change? (check one)
If yes, please describe the impact and migration path for existing applications:
The PR fulfills these requirements:
fix #xxx[,#xxx]
, where "xxx" is the issue number)You have tested in the following browsers: (Providing a detailed version will be better.)
If adding a new feature, the PR's description includes:
To avoid wasting your time, it's best to open a feature request issue first and wait for approval before working on it.
Other information: