Skip to content

fix: add minor fix to allow target being passed to #614

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

Merged

Conversation

drewjbartlett
Copy link
Contributor

This is a minor fix, per @rigor789's suggestion on #612. This works, however, it is not tested since there were not very clear testing guidelines. Also, it's not a very elegant solution. There may be times where you're not aware that an other modal is open, etc. and may not know if you need to pass target. In that case, it's safer to just pass it. Anyway, I'm happy to discuss this further. Thanks!

@rigor789
Copy link
Member

Our testing infrastructure is not really set up properly, the only actual tests are functional tests. With Vue 3 around the corner, we're making testing a priority from the get-go. For now, I'm fine with not having proper tests for this change.

I will have to check if #536 is affected if we are passing a target - as it may have been fixed upstream in NativeScript already. If it has been, I think we can safely revert #558 instead of introducing the target option. Not sure I will have time for this today, so if you have some spare time to test this change, I'd appreciate it!

@rigor789 rigor789 merged commit 31bc425 into nativescript-vue:master Mar 28, 2020
@drewjbartlett drewjbartlett deleted the DB/612-fix-stacking-modal-issue branch March 30, 2020 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants