react-native-popup-menu icon indicating copy to clipboard operation
react-native-popup-menu copied to clipboard

Published 20 hours ago • verified publisher icon instea

Register BackHandler in Menu rather than MenuProvider

Open TheAlmightyBob opened this issue 4 years ago • 7 comments

Fixes #192 by making sure react-native-popup-menu's registers with BackHandler after React Navigation does.

The backHandler property and implementation are left on MenuProvider just to avoid API changes.

I also left the warning about RN<0.44 just because it was easy to copy/paste, but do you really think anybody would try using a new version of this library with a version of React Native before 0.44 at this point...?

TheAlmightyBob avatar Feb 12 '21 03:02 TheAlmightyBob

@sodik82 Are you still around? Could fix this be merged? Or were there concerns about it?

TheAlmightyBob avatar Jul 07 '22 23:07 TheAlmightyBob

Oh dang I hadn't noticed those tests 🤦

I'll sort them out.

(good on you having them and setting up the CI... so many GitHub projects are lacking in that department)

TheAlmightyBob avatar Jul 21 '22 05:07 TheAlmightyBob

thanks for effort... yes, CI are good ;-)

sodik82 avatar Jul 21 '22 15:07 sodik82

Tests should be passing now, and added some new ones while I was there

TheAlmightyBob avatar Aug 15 '22 07:08 TheAlmightyBob

thanks... looks good. I just want to test it a bit to see how does it work when there are multiple menus rendered at one time... ping me in a week if there is no change ;-)

sodik82 avatar Aug 15 '22 12:08 sodik82

Mm, good thinking, I admit that's not a use case of ours so I hadn't considered it.

TheAlmightyBob avatar Aug 15 '22 20:08 TheAlmightyBob

what do you think - isn't it enough to register back handler on menu being open?

sodik82 avatar Aug 15 '22 21:08 sodik82

Hmm. It might be. As long as it's reliably added/removed whenever menus are opened/closed so there aren't stray/duplicate handlers? (regardless of how the open/close was triggered)

It's been a year and half since I looked at this code... does all opening/closing go through MenuProvider's openMenu/closeMenu?

TheAlmightyBob avatar Aug 16 '22 05:08 TheAlmightyBob

to be honest - as I am thinking about it - I don't like registration of backhandler for each menu.... would something like this be fine with you (i.e. would it solve your problem) https://github.com/instea/react-native-popup-menu/pull/251 ?

sodik82 avatar Aug 22 '22 21:08 sodik82

in special situation (special custom backhandler) it is breaking change (since the backhandler is registered only after the menu is opened for the first time)

sodik82 avatar Aug 22 '22 21:08 sodik82

@sodik82 I tried the change in #251 and that seems okay to me. Why was that closed and the branch deleted?

TheAlmightyBob avatar Aug 29 '22 08:08 TheAlmightyBob

shit happens - our migration script overwrite the repo with old version and closed every PR in the process

sodik82 avatar Aug 29 '22 13:08 sodik82