Skip to content

feat: dropdown menu dialog item #168

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ricci2511
Copy link

@ricci2511 ricci2511 commented Apr 14, 2023

Description

While building a project of mine and migrating to your components, I encountered an issue where I needed to have a dropdown where several dropdown items open a different dialog. The workaround mentioned in the notes section of the documentation works for simple cases, but managing the state of a specific dialog programmatically becomes challenging.

Use Case

RadixUI's documentation on the Dialog component has a good example of such a use case, where the dialog needs to close after an asynchronous form submission, example.

Changes Made

I have created a new component that combines the Dropdown and Dialog components to address this issue. Since this component is used within a dropdown menu, I added it to @/components/ui/dropdown-menu.tsx.

Also the example I've addded is rather simple, so let me know if I have to tweak it to show a more "complex" example like the asynchronous form submission thing I mentioned earlier.

@vercel
Copy link

vercel bot commented Apr 14, 2023

@ricci2511 is attempting to deploy a commit to the shadcn-pro Team on Vercel.

A member of the Team first needs to authorize it.

@ricci2511
Copy link
Author

ricci2511 commented Apr 15, 2023

After some consideration, It might make sense to refactor some of the types and props in DropdownMenuDialogItem. Since the base forwardRef generic types are the same as for DropdownMenuItem, it could be useful to reuse them as follows:

type DropdownMenuItemElementRef = React.ElementRef<
    typeof DropdownMenuPrimitive.Item
>;
type DropdownMenuItemPropsWithoutRef = React.ComponentPropsWithoutRef<
    typeof DropdownMenuPrimitive.Item
>;

Besides that, some of the additional props I've added in DropdownMenuDialogItem could be streamlined. The children prop is redundant and the onSelect type could be updated to match the actual type of the DropdownMenuItem component, which is ((event: Event) => void) | undefined.

So finally the result might look something like this:

const DropdownMenuDialogItem = React.forwardRef<
    DropdownMenuItemElementRef,
    DropdownMenuItemPropsWithoutRef & {
        trigger: React.ReactNode;
        open?: boolean;
        onOpenChange?: DialogProps['onOpenChange'];
        onSelect?: DropdownMenuPrimitive.DropdownMenuItemProps['onSelect'];
    }
>(({ trigger, children, open, onOpenChange, onSelect, ...props }, ref) => {
    return (
        <Dialog open={open} onOpenChange={onOpenChange}>
            <DialogTrigger asChild>
                <DropdownMenuItem
                    ref={ref}
                    onSelect={(event) => {
                        event.preventDefault();
                        onSelect && onSelect(event);
                    }}
                    {...props}
                >
                    {trigger}
                </DropdownMenuItem>
            </DialogTrigger>
            {children}
        </Dialog>
    );
});
DropdownMenuDialogItem.displayName = 'DropdownMenuDialogItem';

@shadcn
Copy link
Collaborator

shadcn commented Sep 18, 2023

@ricci2511 do you think this should be part of the primitive? Could this be better as an example? Thank you.

@shadcn
Copy link
Collaborator

shadcn commented Mar 27, 2025

This PR has been automatically marked as stale due to one year of inactivity. It will be closed in 7 days unless there’s further input. If you believe this PR is still relevant, please leave a comment or provide updated details. Thank you.

@shadcn shadcn added the stale? Is this issue stale? label Mar 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale? Is this issue stale?
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants