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

fix(ios): properly fire close event for iOS 13+ modal windows #11135

Closed
wants to merge 1 commit into from

Conversation

hansemannn
Copy link
Collaborator

@build build added this to the 8.2.0 milestone Aug 12, 2019
@build build requested a review from a team August 12, 2019 15:38
@build
Copy link
Contributor

build commented Aug 12, 2019

Fails
🚫

🔬 There are library changes, but no changes to the unit tests. That's OK as long as you're refactoring existing code, but will require an admin to merge this PR. Please see README.md#unit-tests for docs on unit testing.

🚫

Test suite crashed on iOS simulator. Please see the crash log for more details.

Warnings
⚠️

🔍 Can't find junit reports at ./junit.*.xml, skipping generating JUnit Report.

Messages
📖 🎉 Another contribution from our awesome community member, hansemannn! Thanks again for helping us make Titanium SDK better. 👍
📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.

Generated by 🚫 dangerJS against 7b04719

@hansemannn
Copy link
Collaborator Author

It seems like this crashes with the test suite but I cannot access the logs. I thought that no Xcode 11 guards are required since it's only a new delegate function which part of an existing delegate and won't be called on earlier versions, but it may does cause issues!

Copy link
Contributor

@vijaysingh-axway vijaysingh-axway left a comment

Choose a reason for hiding this comment

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

@hansemannn It is crashing when opening and closing window multiple times. See PR #11057. In this PR, this issue is already fixed.

@hansemannn
Copy link
Collaborator Author

@vijaysingh-axway Is that really the same thing? Your PR is for configuring whether or not a modal window should be swipeable, this one is a bugfix for windows that don't trigger a close event, causing huge glitches in depending apps. I see you introduced a new flag for it, so if it works for this one as well, feel free to cherry-pick and integrate my changes into your PR.

@vijaysingh-axway
Copy link
Contributor

@vijaysingh-axway Is that really the same thing? Your PR is for configuring whether or not a modal window should be swipeable, this one is a bugfix for windows that don't trigger a close event, causing huge glitches in depending apps. I see you introduced a new flag for it, so if it works for this one as well, feel free to cherry-pick and integrate my changes into your PR.

@hansemannn What I mean was, In my PR #11057, 'close' event handling for iOS 13+ modal windows is already there . I have verified your test case. So we do not need this PR. It would be great, if you can test at your end. Thanks!

@hansemannn
Copy link
Collaborator Author

Not required anymore.

@hansemannn hansemannn closed this Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants