-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat: Add CollectionNode
to collection renderer
#8523
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
base: main
Are you sure you want to change the base?
Conversation
@devongovett Would love to hear your thoughts on this. For more context, this PR relates to #8317 (comment). Let me know if the docs need updates as well here! |
So you are looking for some kind of "inheritance" for collection renderers basically? Like use virtualizer but add an extra wrapper element around items? Can you describe in a bit more detail how your use case (a carousel I guess?) works? One concern is that it could be somewhat easy to mess up the DOM structure expected by an existing renderer, but maybe it's an advanced use case anyway so you know the risks. I was going to say the default collection renderer was pretty simple so it wouldn't be too hard to copy, but the drop indicator stuff is kinda complicated now so maybe this makes sense. Do you usually want to wrap just the item itself or also wrap the drop indicators? Sorry for all the questions just trying to think through the use cases here. |
@devongovett Yes, thats right! The primary idea for this PR was to be able to create a component similar to PS: We have already moved on from this implementation to one based on a |
Got it, thanks. I think something like that makes sense but I haven't fully thought it through yet. We're in release mode at the moment so responses might be slow. Will discuss it with the team soon. |
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 discussed this as a team and we think this is a reasonable API addition. Just one question on prop naming in case you have some ideas.
/** The content that should be rendered before the item. */ | ||
before?: ReactNode, | ||
/** The content that should be rendered after the item. */ | ||
after?: ReactNode |
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.
Do you think we should name these props so they are more specific to drop indicators? Not sure if we might want to add other things between items in the future, whether we'd want to group them here or allow them to be passed in separately for more control.
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 was questioning the same while working on this. Personally, I do feel like a user of this feature is looking for as much control here as possible. A compromise between the two could maybe be to change the signature to ReactNode[]
or ReactElement[]
to make destructuring much easier.
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.
@devongovett Btw, since I saw your reply on X, we might want to offer a wrapper
prop here as well to wrap the node. Maybe thats easier than cloning the node just to modify its render function.
This adds an optional
CollectionNode
component to the collection renderer interface, which enables customization of a nodes render output without having to re-implement the entire renderer. This can be useful to build wrapper components which modify a nodes pseudo content without breaking an existing custom renderer, e.g.<Virtualizer />
.✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: