chore: migrate autocomplete component #21323
Conversation
| getOptionLabel={(option) => option.email} | ||
| onOpen={() => { | ||
| setOpen(true); | ||
| setFilter(value?.email ?? ""); |
There was a problem hiding this comment.
Im not sure why setFilter was previously set to value?.email, this made it very difficult to choose a different value for the owner on the Create workspace page
There was a problem hiding this comment.
For example, try out the existing owner autocomplete dropdown vs the one in this PR, I cant think of a reason why the existing one is difficult to use.
418bd76 to
d99aee4
Compare
| * Container element to portal the dropdown into. Use this when rendering | ||
| * inside a Dialog to avoid scroll lock issues. | ||
| */ | ||
| popoverContainer?: HTMLElement | null; |
There was a problem hiding this comment.
passing an element as a prop feels like a smell to me. I notice tho that the one place we're using this is to pull an element out of the ref.
can we just pass the ref instead?
| if (isOpen) { | ||
| setFilter(""); | ||
| } else { | ||
| setFilter(undefined); | ||
| } |
There was a problem hiding this comment.
you could also just do setFilter(isOpen ? "" : undefined)
| <Avatar | ||
| src={option.created_by.avatar_url} | ||
| fallback={option.name} | ||
| <Dialog open={open} onOpenChange={(isOpen) => !isOpen && onClose()}> |
There was a problem hiding this comment.
I really am not a fan of the readability of using && short circuiting this way, and would rather just make this an if
| getOptionValue={(option) => option.name} | ||
| getOptionLabel={(option) => option.name} | ||
| renderOption={(option) => ( | ||
| <span className="flex-1">{`${option.flag} ${option.name}`}</span> |
There was a problem hiding this comment.
| <span className="flex-1">{`${option.flag} ${option.name}`}</span> | |
| <span className="flex-1">{option.flag} {option.name}</span> |
| <span className="flex-1">{`${option.flag} ${option.name}`}</span> | ||
| )} | ||
| label={Language.countryLabel} | ||
| placeholder={"Select a country"} |
| export const ChangeWorkspaceVersionDialog: FC< | ||
| ChangeWorkspaceVersionDialogProps | ||
| > = ({ workspace, onClose, onConfirm, ...dialogProps }) => { | ||
| > = ({ open, workspace, onClose, onConfirm }) => { |
There was a problem hiding this comment.
Any reason we wouldn't want the rest of these ...dialogProps here?
Also, adds built-in required and error text