-
Notifications
You must be signed in to change notification settings - Fork 22.5k
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
Remove slottable mixin #2216
Remove slottable mixin #2216
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.
Again, looking good Mr Scholz.
A few points:
-
Surely
/en-US/docs/Web/API/Slottable
should redirect to/en-US/docs/Web/API/Element
? It currently redirects to nowhere. -
Same comment as for the other review about possibly providing a note in the spec table to say that the property is in fact defined on a mixin, just in case?
-
Text/assignedSlot
shouldn't have the experimental banner at the top of it.
done by doing
Hm, I'm not sure about this one. I think we want to autogenerate the spec section from BCD one day (without comments) and also if you are wondering about mixins you are likely already familiar with the spec?
Removed :) |
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.
Yay, all looks good to me!
I don't feel that strongly about the mixin note thing; happy to leave that alone. You are right that it would make auto-generation harder.
Analog to mdn/browser-compat-data#9048
This time,
yarn content move
worked as expected. I might have forgotten to run "yarn" from my content folder in the other mixin PR (it is easy to miss).Anyway, here's another mixin to avoid. For the
Text
interface it was mostly correct as is already.I made a few sensible commits which could be preserved (or not, your choice). It didn't make sense to me to put the steps in separate PRs.