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
Implement custom content typing #3298
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 60778d2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 60778d2:
|
Thanks for implementing this. I'm not entirely sure about this API, though. I'd like to explore more alternatives to see if they mesh better with other use cases (when they come up). For instance, the lookup text is currently obtained from the item object using const defaultItems = [
{ value: "Apple", children: "🍎 Apple" },
// ...
];
<SelectProvider defaultItems={defaultItems}>
<Select />
<SelectPopover>
<SelectRenderer>
{(item) => <SelectItem key={item.id} {...item} />}
</SelectRenderer>
</SelectPopover>
</SelectProvider> I wonder whether we could use a new property: <SelectPopover>
<SelectItem typeaheadText="Apple" value="Apple">
🍎 Apple
</SelectItem>
</SelectPopover> This property would be registered along with the item in the composite store and used by item.typeaheadText || item.element?.textContent || item.children This is just an idea. I'm not entirely convinced about it either. |
Hey @diegohaz I really loved your idea of implementation, in fact the need for repeating the same logic for I will implement the recommendations, and then we can revisit the process for further improvement. |
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 looks good, thanks a lot!
I'll try to gather a few more use cases to see if this API makes sense.
@@ -194,6 +195,7 @@ export const useCompositeItem = createHook<CompositeItemOptions>( | |||
id: id || item.id, | |||
rowId, | |||
disabled: !!trulyDisabled, | |||
typeaheadText: typeaheadText, |
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.
typeaheadText
is missing in the callback deps. We should probably add the linter to the project as it could help identify existing issues like that.
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.
Yeah, I agree.
Could we include this lint in the following PR? I've done a few tests, and there are some things we must address in other files too.
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, it's better as a separate PR.
Closes #2699