nandorojo / zeego

Menus for React (Native) done right.
https://zeego.dev
MIT License
1.42k stars 41 forks source link

Cannot create wrapper around DropdownMenu.ItemIcon #50

Closed Braden1996 closed 8 months ago

Braden1996 commented 12 months ago

Hello!

I am trying to create a reusable configuration of DropdownMenu.ItemIcon to indicate a checkbox state (I see there is DropdownMenuCheckboxItem, but we want a different look).

I've seen in the docs that I need to create a custom component, but the following doesn't seem to work.

interface DropdownMenuItemCheckIconComponentProps
    extends Omit<React.ComponentProps<typeof DropdownMenu.ItemIcon>, "ios" | "androidIconName"> {
    checked: boolean
}

export const DropdownMenuCheckIcon = DropdownMenu.create(
    ({checked, ...rest}: DropdownMenuItemCheckIconComponentProps) => {
        const theme = useTheme()

        return (
            <DropdownMenu.ItemIcon
                {...rest}
                {...(checked && {
                    ios: {
                        name: "checkmark.circle.fill" as const,
                        paletteColors: [theme.tokens.accent],
                    },
                })}
            />
        )
    },
    "ItemIcon"
)

Any ideas? :)

nandorojo commented 12 months ago

you should use the ItemIndicator. the iOS check gets added automatically.

https://zeego.dev/components/dropdown-menu#itemindicator

nandorojo commented 12 months ago

I see that you’re saying you want a different look. I recommend staying within the iOS guidelines. If you want to still customize the behavior with your own check, which I don’t recommend, you’ll have to write the icon name inline when you render the component. But this isn’t recommended.

Braden1996 commented 12 months ago

I see that you’re saying you want a different look. I recommend staying within the iOS guidelines. If you want to still customize the behavior with your own check, which I don’t recommend, you’ll have to write the icon name inline when you render the component. But this isn’t recommended.

Our Checkbox case isn't conventional to the guidelines, maybe we'll change it later on. I suppose this is more a question on whether it's possible to create reusable prop configurations of Dropdown Menu components. Is there a mechanism for doing this? CleanShot 2023-07-05 at 16 54 52

nandorojo commented 12 months ago

as for the reusable nature…that’s a totally fair desire. unfortunately, due to the mechanics of how Zeego works (it uses some magic to read props and give them to the native side), this doesn’t quite work. you need to render the props themselves inline for native. i might be able to add a PR that uses defaultProps or something, but i’m not 100% sure if it’ll work

MaxAst commented 11 months ago

I've run into a similar issue, where I want to add a onSelect in my custom component, but it doesn't seem to work. Here's what I got:

const CancelDropdownMenuItem = DropdownMenu.create((props: Props) => {
  return <DropdownMenuItem {...props} onSelect={() => Alert.alert("hey")} />;
}, "Item");

and

<DropdownMenuRoot>
    <DropdownMenuContent>
        <CancelDropdownMenuItem destructive key="cancel">
          <DropdownMenuItemTitle>Cancel</DropdownMenuItemTitle>
          <DropdownMenuItemIcon
            ios={{
              name: "x.circle",
              scale: "default",
            }}
          />
        </CancelDropdownMenuItem>
    </DropdownMenuContent>
</DropdownMenuRoot>

Like this, the alert is never triggered, but it does trigger when I move it inline to <CancelDropdownMenuItem destructive key="cancel" onSelect={() => Alert.alert("hey")}>.

Is this expected or am I missing something?

nandorojo commented 10 months ago

This is as expected. You need to have the triggers inline, this is a limitation of the native side. What you did would only work on Web.

nandorojo commented 8 months ago

Hey @MaxAst, I think I'm going to close this out. I agree that it's a bit annoying to not be able to fix this – for example, it would be useful to have things like a <DropdownMenu.LinkItem href="/" />.

However, due to the workarounds used to make this work gracefully on native, we aren't able to wrap the components easily. The components themselves are simply a glorified array that's getting looped through. Maybe I could think of some additional abstraction in the future to support this, but for now it's not totally clear how it would work.

MaxAst commented 8 months ago

all good @nandorojo, thanks a lot for creating this amazing library!