search

nandorojo / zeego

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

feat: New prop for supporting onWillShow and onWillHide props in iOS #25

Closed arvidnilber closed 1 year ago

arvidnilber commented 1 year ago

Added new prop called onExpandedChange which does the callback earlier than onOpenChange, which fixes issues with for example rotating an arrow depending on the expanded state. See videos below for example

It uses the same API design as onOpenChange.

Using new prop onExpandedChange

https://user-images.githubusercontent.com/6342534/197208109-72d2e365-e8a1-46d3-9396-92b79741ed8c.mp4

Using onOpenChange

https://user-images.githubusercontent.com/6342534/197208490-be14fd0c-95ad-459d-bd5f-60ff07a9c21c.mp4

Discussion:

Not sure if the naming is that good, feels like onOpenChange is a better suited name for this callback, and to rename the current onOpenChange to onOpenedChange, but that will cause breaking changes.

vercel[bot] commented 1 year ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
zeego ❌ Failed (Inspect) Dec 10, 2022 at 0:08AM (UTC)
zeego-docs ❌ Failed (Inspect) Dec 10, 2022 at 0:08AM (UTC)
xmflsct commented 1 year ago

When can this be merged?

nandorojo commented 1 year ago

The current prop names are using Radix’s API, but it doesn’t look like this prop exists in Radix. So maybe we only add it to native with onWillChange? Also, does react-native-menu support this? If so, we can add it on Android too.

arvidnilber commented 1 year ago

The current prop names are using Radix’s API, but it doesn’t look like this prop exists in Radix. So maybe we only add it to native with onWillChange? Also, does react-native-menu support this? If so, we can add it on Android too.

onWillChangesounds better, I will change it to that. Sadly, I couldn't find any support for it in react-native-menu, so no Android support yet.

nandorojo commented 1 year ago

I made some small changes to the naming and prop types. Would you mind adding this to the docs for context and dropdown menus? It just needs to get added to the props table. After that, good to go.

ouwargui commented 1 year ago

@nandorojo I openned a new pull request https://github.com/nandorojo/zeego/pull/38 with the docs updated so we can move on with this feature.