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

Modal onDismiss support for Android #769

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bernhardoj
Copy link

This PR proposes to add Modal onDismiss support for the Android platform.

A rendered version of the proposal can be read here

@bernhardoj
Copy link
Author

For context, we previously had a PR to add the onDismiss support for Android, but it was reverted due to an internal codebase that relies on the event not being fired.

With this proposal, I hope we can discuss how to add the onDismiss support for Android properly.

cc: @cipolleschi

@cortinico
Copy link
Member

but it was reverted due to an internal codebase that relies on the event not being fired.

That's correct. I believe we won't be able to easily implement onDismiss for Android as we already have apps out in the wild abusing this lack of implementation. Potentially the best way forward is to create a new onModalDismiss property and deprecate the onDismiss one + with a proper migration path.

Just to set expectation, @cipolleschi will be off for the whole month of March. This RFC can't move forward without a champion from Meta, and I won't be able to take on this work as we're currently already 100% full rolling out 0.74 so we'll have to wait to move this forward, sorry for that.

@bernhardoj
Copy link
Author

Potentially the best way forward is to create a new onModalDismiss property and deprecate the onDismiss one + with a proper migration path.

That's a good idea to prevent breaking changes. I was thinking that perhaps we can add an enableDismissAndroid props, but this would just add more platform-specific props.

This RFC can't move forward without a champion from Meta, and I won't be able to take on this work as we're currently already 100% full rolling out 0.74 so we'll have to wait to move this forward, sorry for that.

Sorry, I'm not familiar with the process. Do you mean I need to wait for the other Meta member to lead this RFC or you are the one who will lead it but will wait until the 0.74 release is done?

@cortinico

@cortinico
Copy link
Member

Do you mean I need to wait for the other Meta member to lead this RFC or you are the one who will lead it but will wait until the 0.74 release is done?

We'd need to find someone from Meta that is interested in driving this forward. Both @cipolleschi and me are essentially off till mid April. We could resume the work on this front afterwards

@bernhardoj
Copy link
Author

I see. Thanks for the clarification.

@shubham1206agra
Copy link

@cortinico @cipolleschi Are you back from OOO?
Can we push this proposal now?

@shubham1206agra
Copy link

shubham1206agra commented Jul 3, 2024

@cortinico @cipolleschi Can you please look into this?

Copy link
Member

@cortinico cortinico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left my feedback here.

I'd like also to point out how both @cipolleschi and me are hyperfocused on New Architecture rollout at the moment. This falls out of this domain so we won't be able to drive it in the immediate future (sorry for that but our capacity is extremely limited)

Comment on lines +58 to +59
- It could be a breaking change if an app expects the Android Modal never fires `onDismiss` event, for example:
The code uses `onDismiss` to listen for modal dismiss event on iOS and uses another (workaround) approach on Android. If the `onDismiss` is starting to fire on Android too, they would receive two dismiss events.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is practically a hard blocker for us.

IIRC @cipolleschi already imported a change that was implementing onDismiss on Android, as we had several apps that relied on the 'lack' of Android implementation and started misbehaving.

I believe we won't be able to ship this as it is, but we'll have to:

  1. Deprecate onDismiss
  2. Introduce a new onModalDismiss event that is fired by both platform

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bernhardoj Please make changes.

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

Successfully merging this pull request may close these issues.

3 participants