Skip to content
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

Proposal for Android event type changes #402

Closed
MoOx opened this issue Mar 1, 2021 · 1 comment
Closed

Proposal for Android event type changes #402

MoOx opened this issue Mar 1, 2021 · 1 comment

Comments

@MoOx
Copy link

MoOx commented Mar 1, 2021

Feature request

I was about to create another issue, but the topic is similar so trying here...

  • Why AndroidEvent does have the (undocumented, but very useful) type outside of nativeEvent? Is there any practical reason I am missing?
  • Could we imagine a transition with this type in nativeEvent data (with a deprecation period to ensure no brutal breaking change)?

Also, I didn't find the event type in the readme (except for neutralButtonPressed).
The most interesting (at least in my case) is "dismissed" value, especially if you think about having inline picker on iOS 13+ a classic modal for android (where you need to listen to dismiss (cancelled) event).

Why it is needed

I was about to update bindings for ReScript but the way nativeEvent are designed for ReScript (still named @reason-react-native/datepicker) make it not simple to update them for values outside of nativeEvent payload (I didn't say hard, I can do this but I doesn't feel right as all the lib I know use "custom data" inside nativeEvent.

Possible implementation

We could duplicate type inside nativeEvent (and keep it outside with a warning when accessed) to make it more normalized with a transition period.

Code sample

In practise, we could just use event.nativeEvent.type instead of event.type.

I am open to make this non immediately breaking change

@vonovak
Copy link
Member

vonovak commented Mar 13, 2022

hello @MoOx,
sorry for taking a year to answer 😆
The reason type is included in the event is, as you pointed out, to distinguish what operation user performed on android.

It's true that the property is not documented, simply because I didn't do it for reasons explained in #313 and no one else did it, either.

However, the sponsorship situation of this package has been getting better and documentation improvements are coming :)

I cannot say why the type lives outside of nativeEvent because it was done this way before I got involved but if you believe the better place for it is in the nativeEvent then I'm open to merging a PR that does this in a non-breaking way :)

Thank you 🙂

@vonovak vonovak closed this as completed Oct 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants