-
Notifications
You must be signed in to change notification settings - Fork 150
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
Add support for modals in android #17
Add support for modals in android #17
Conversation
} | ||
|
||
return; | ||
} | ||
|
||
if (view == null) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check should be moved up before view.hasWindowFocus
@@ -73,4 +92,19 @@ public void onClick(View v) { | |||
|
|||
snackbar.show(); | |||
} | |||
|
|||
private void recursiveLoopChildren(ViewGroup view) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this need to be recursive, in case modals call other modals?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, and we can have more than one modal. :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, seems like it should work. I'm also curious why the snackbar doesn't appear on top of the modal by default, does React Native do something sneaky to make modals appear? A link or something would be nice if you happen to know. I'm just curious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, the modals in RN for Android are dialogs that are always displayed above the activities. It does not work for that reason.
Hi @SandroMachado, thanks for this! Could you please post some example JS code so I can test this out myself? I'll try to get to it later this week. |
Sure, for instance this code in Android:
|
@SandroMachado have a look at 3168d8d -- I implemented a translucent modal, and the snackbar still shows up underneath it (only on Android) even with this PR. It looks like this is actually creating multiple snackbars, one on top of each modal, is that correct? I wonder if there's a better way to just get the root view of the whole activity and put the snackbar there. I'll debug further later this week, until then please let me know if you have any ideas! |
@cooperka I have decided to add to all the modals, because we can have more than one modal. I didn't find a way to discover which one is visible. Unfortunately, this is not the perfect solution, but, since all the Probably, we can open a new issue and try to fix it in the future. |
I'm definitely okay with this solution for now at least, since it's an uncommon scenario anyway. Could you please have a look at the example I linked to and see if it's also broken for you? |
@cooperka I tested on my device, and I think it is working as expected. Or I am missing something? |
@cooperka sorry to bother you again. Do you have an ETA to merge this PR and make a release? |
@SandroMachado sorry for the delay, unfortunately I can't give much of an ETA but I will try to get to it as soon as I can. I was hoping to merge this weekend but a few other things came up. For now I hope you can use your fork until I can test again and merge. On my machine the snackbar was appearing underneath the red modal; I'm not sure if it's related to Android version or maybe I just installed incorrectly... I did run |
Thanks again for your patience. I spun up my machine again and cleared caches, this time it worked. I appreciate the PRs, I'll publish a new version momentarily :) |
@cooperka thanks for the release 👍, back to upstream :) |
Closes #16.