Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[slottable-request] Initial draft #45
[slottable-request] Initial draft #45
Changes from 5 commits
f7f7f6e
8e1ee6b
b0397fd
47bbb7e
d3d1a2f
e060b07
12df8ee
9a5e98e
05c0476
85530e8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This virtual list does not use this pattern: https://github.com/WICG/virtual-scroller
Worth reviewing why they came to the decision not to.
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 virtual-scroller effort is defunct, but even so it did seem to follow a very similar pattern where the container fires an event to tell the host to render new content: https://github.com/WICG/virtual-scroller#rangechange-event
Can you say what you think the important differences are?
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.
It's unclear to me how the event-listener is intended to provide the content. Inline examples would help clarify this for me.
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.
Given that
remove
is a short, common identifier, we may want to consider exposing this as a static symbol likeSlottableRequestEvent.remove
.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.
What happens if the component is defined before the parent component has been defined? It will miss these events.
One solution here is to have an imperative API, such as a function that you can call, to cause a render to occur.
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.
Could you explain who would call the imperative API, and how they would know call it?
Slotting is really a form of dependency injection, and it does get tricky when the owning scope isn't available to provide dependencies. We've dealt with this in the context protocol at an implementation level by having an "unfulfilled context request" manager at the root re-dispatch context requests from consumers to providers see discussion. We haven't updated the protocol to include that yet, but we implement it in Lit's version to pretty good effect, and I could see something like that forming a solution.
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.
It's unclear to me how the event-listener is intended to provide the content. Inline examples would help clarify this for me.
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.
Code examples here would be good.
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 playground links are code examples -- were you looking for inline code snippets? They got a bit long and distracting so moved them to playground.
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, outside links are fine too, but linkrot exists so there should be inline examples to view, and having a condensed version is a good exercise to see if the API scales down to simpler cases.
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, all of these examples are using a framework, so it's harder to tell what is going on unless you understand the internals of what the framework is doing. So one or two plain JS examples would be good.