- Notifications
You must be signed in to change notification settings - Fork926
chore: add DropdownMenu component#15941
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
| }; | ||
| DropdownMenuShortcut.displayName = "DropdownMenuShortcut"; | ||
| export { |
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.
There is a preference to not place all exports at the bottom of the file.
| @@ -0,0 +1,198 @@ | |||
| import * as DropdownMenuPrimitive from "@radix-ui/react-dropdown-menu"; | |||
| import { Check, ChevronRight, Circle } from "lucide-react"; | |||
| import * as React from "react"; | |||
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 have been following the pattern of not using the * import for React for the other shadcn/ui components as we don't generally have this pattern in Coder
| <DropdownMenuPrimitive.SubContent | ||
| ref={ref} | ||
| className={cn( | ||
| "z-50 min-w-[8rem] overflow-hidden rounded-md border border-solid bg-surface-primary p-1 text-content-secondary shadow-lg data-[state=open]:animate-in data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=open]:fade-in-0 data-[state=closed]:zoom-out-95 data-[state=open]:zoom-in-95 data-[side=bottom]:slide-in-from-top-2 data-[side=left]:slide-in-from-right-2 data-[side=right]:slide-in-from-left-2 data-[side=top]:slide-in-from-bottom-2", |
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.
For the other shadcn/ui components I have been converting very long classNames to multi line strings, roughly grouped by the general types of classes. This make it easier to read.
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, we are not currently using the classname sorting
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 should definitely usecn if we want to create extendable components. It really helps in figuring out class hierarchies and merging similar styles.
The function cn is a helper utility that makes it easier to handle className manipulation in a React + Tailwind environment, ensuring that classNames are appropriately merged and any conflicts are resolved, making the component styling more consistent and maintainable.
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.
Ah, I meant the biome classname sorting that just sorts the order the utility classes appear in the code.
| @@ -0,0 +1,53 @@ | |||
| /** | |||
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 think this comment should be on the component instead of the stories.tsx
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.
Totally! Idk why I put it in the stories 🤦
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.
Just let a few comments about the style to match how to the other shadcn components have been integrated. Otherwise looks good.
809c611 intomainUh oh!
There was an error while loading.Please reload this page.
Related to#15617