-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat: add radix autocomplete component #21262
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
Conversation
jaaydenh
commented
Dec 13, 2025
| className="w-[var(--radix-popover-trigger-width)] p-0" | ||
| align="start" | ||
| > | ||
| <Command shouldFilter={controlledInputValue === undefined}> |
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.
how would this react to "" as a value?
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.
empty string is still considered a controlled input value so shouldFilter will be false and it will be assumed the options provided by the parent component have already been filtered
| * Type guard to check if the value is a Group. | ||
| * Groups have a "members" property that users don't have. | ||
| */ | ||
| export const isGroup = ( |
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.
big fan of isolating this logic, but maybe modules/groups/ or something could be a better spot? I'm not a fan of adding new code to utils/
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.
Moved the entire groups.ts file to modules/groups.ts, if we are moving away from the utils folder, we should decide on a pattern and document it in the frontend contributing guide. Should the rule be that code that is shared across multiple sub-folders in the pages directory should live in the root of modules instead of utils?
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 like to think of organizing more in terms of "what problem the code solves" rather than "what the code is". sure, it might be a utility function, but it's a utility function that is related to how our sharing implementation works for templates and workspaces.
I think documenting that is a good idea
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 like having a groups file contain all of the groups related helpers even if they are helping solve different problems. I think it would make more sense to split into separate files if for example there were 3 related helpers solving 1 specific problem.