nandorojo / zeego

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

[Android] DropdownMenu not working #5

Closed thespacemanatee closed 2 years ago

thespacemanatee commented 2 years ago

Hello! Thanks for this cool library!

We just tried a small snippet from the example project and found that it only works on iOS (and maybe web), but on Android, nothing is being shown when I tap on the Trigger. I this a known issue because I see that Android is supported from the documentation? Thank you!

Repro

 <DropdownMenu.Root key={filter}>
    <DropdownMenu.Trigger>
      <ThemedChip
        key={filter}
        isSelected
        label={filter}
        style={tw('mx-2 my-2')}
        rightComponent={
          <ThemedIcon
            name="x"
            color="white"
            size={16}
            style={tw('ml-1')}
          />
        }
        // onPress={() => removeFilter(filter)}
      />
    </DropdownMenu.Trigger>
    <DropdownMenu.Content
      style={{
        minWidth: 220,
        backgroundColor: 'white',
        borderRadius: 6,
        padding: 5,
        borderWidth: 1,
        borderColor: '#fff8',
      }}
    >
      <DropdownMenuLabel>Help</DropdownMenuLabel>

      {[1, 2, 3].map(i => (
        <DropdownMenuItem key={`list-${i}`}>
          <DropdownMenuItemTitle>{`Item ${i}`}</DropdownMenuItemTitle>
        </DropdownMenuItem>
      ))}
    </DropdownMenu.Content>
  </DropdownMenu.Root>
nandorojo commented 2 years ago

hey, thanks for checking! android is on the TODO at the bottom, though i thought it worked. does it just render nothing?

nandorojo commented 2 years ago

it should be a JS version only. i assume you installed the peer deps?

thespacemanatee commented 2 years ago

@nandorojo thanks for the prompt response! Yes, I have made sure to install all peer deps. On Android, it renders nothing when I tap on a View that is wrapped by the <DropdownMenu.Trigger /> from the example code.

EDIT: Added repro in the OP

nandorojo commented 2 years ago

@axeldelafosse @intergalacticspacehighway were planning on adding the native menu support to Android, it should work after that is added.

i don't own an android so i'm sadly not exactly able to reproduce this one

nandorojo commented 2 years ago

to be sure, can you just put a text node inside of the trigger?

thespacemanatee commented 2 years ago

@nandorojo Using a Text node instead works! Does this mean that I have to menunify my custom components for it to work on Android?

nandorojo commented 2 years ago

ahh my guess is that your ThemedChip isn't forwarding down the ref and the onPress prop. try that.

this is only necessary for android (for now)

thespacemanatee commented 2 years ago

@nandorojo sorry I realised I should have tried debugging more first before replying, but I found that it actually did work (somewhat), but this is what it looks like after I applied some debugging styles (padding and backgroundColor: "black"):

photo_2022-06-13_09-38-47

I am using a Pressable component in my ThemedChip which is stealing touch, and it works if I replace it with a View. Could you give an example of how I could forward the ref from Trigger to my Pressable component? Apologies as I am not very familiar with that 😔 Thank you!

intergalacticspacehighway commented 2 years ago

@thespacemanatee can you remove Pressable from ThemedChip and just use a View? Zeego also adds a Pressable on Trigger (on Android) so it won't work well with your Pressable. I am working on adding a native menu for android, which will solve it in the future.

nandorojo commented 2 years ago

As for forwarding the ref, it should look like this:

import { forwardRef } from 'react'

export const ThemedChip = forwardRef((props, ref) => {
  return <View ref={ref}>{...}</View>
})
thespacemanatee commented 2 years ago

@nandorojo thanks for the example. I am familiar with that and did try it, but wasn't very sure what it does since DropdownMenu.Trigger doesn't seem to be forwarding any ref to its children.

Simply using a View instead of a Pressable works for the moment, though I can't implement any onPressed styles.

nandorojo commented 2 years ago

Yeah, there is some magic going on here for Android. Under the hood, it's cloning the child and passing down a ref.

It first extracts the Trigger child here: https://github.com/nandorojo/zeego/blob/master/packages/zeego/src/menu/create-android-menu/index.android.tsx#L119

Update: now that I'm looking at it again, I don't think forward ref should be necessary, since we're measuring the Pressable: https://github.com/nandorojo/zeego/blob/master/packages/zeego/src/menu/create-android-menu/index.android.tsx#L149-L156

So the only thing that really matters is not using a Pressable I believe. This will be fixed when we move to a native menu.

thespacemanatee commented 2 years ago

Thank you guys for the informative discussion, and since there isn't an 'issue' with the Android implementation and a better implementation is coming up, feel free to close this issue! 😃

nandorojo commented 2 years ago

The Android native menu is now live in version 0.4.0. It requires installing @react-native-menu/menu.

Shout out to @intergalacticspacehighway for the #6 PR

ddikodroid commented 2 years ago

@nandorojo there is a "good" popup menu on android. maybe you can take a look at it

https://github.com/saket/cascade

nandorojo commented 2 years ago

@ddikodroid thanks for sharing! that's a pretty menu.

using the best native library for a menu is very attractive. i'll admit that it's a bit unlikely that i'm able to add that, since i don't even know Java / native code itself, and i worry about maintaining something so out of my reach. i'd prefer to keep the API surface area within using RN libraries.

maybe there could be some sort of binding for these extra cases, like zeego/dropdown-menu/cascade. it's hard to say the best way to do it

anyway, i'll keep this in mind. appreciate the suggestion.