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

GH-506 ios: This should fix the issue based on litiobat comment. #527

Closed
wants to merge 1 commit into from

Conversation

riddletd
Copy link

@riddletd riddletd commented Nov 19, 2019

Platforms affected

iOS

Motivation and Context

#506

Description

I updated resultForVideo() with the solution provided in issue #506 by litiobat.

Testing

I have tested this code on iPhone 7 Plus with iOS 13.1.3.

Checklist

  • I've run the tests to see all new and existing tests pass
  • I added automated test coverage as appropriate for this change
  • Commit is prefixed with (platform) if this change only applies to one platform (e.g. (android))
  • If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
  • I've updated the documentation if necessary

@asztal
Copy link

asztal commented Nov 20, 2019

If it helps anyone, I have tested this exact commit in my app and it works as expected.

It's worth noting that my iOS simulator was never affected, this only ever happened for me on real devices.

@riddletd
Copy link
Author

Thank you, @asztal. I edited my first comment accordingly. I have been doing a lot of testing on real and simulated iPhones for a specific application which led me to think I tested this plugin on a simulator as well. Although, I did not test this specific plugin on a simulated device. My mistake. Thank you for catching that.

@JakeKaad
Copy link

JakeKaad commented Apr 2, 2020

@riddletd This fixes the issue for us. Thank you. Do you know what it would take to get your code merged?

@riddletd
Copy link
Author

riddletd commented Apr 2, 2020

@JakeKaad I'm glad it helped you. It fixed the issue for my team as well. I'm not sure how to get it merged. That's why it has been sitting here as a PR for so long.

I did a little research on how to merge this a while back, and I read that it was my responsibility to reach out and get the attention of a cordova maintainer. I didn't know who to or how to do that.

If you have any suggestions or could provide help in any way, I am all ears.

Thanks.

@JakeKaad
Copy link

JakeKaad commented Apr 2, 2020

@riddletd I'm not sure. Is your team using a forked version of this plugin for now, or are you just pointing at your branch?

@riddletd
Copy link
Author

riddletd commented Apr 2, 2020

@JakeKaad Yes. I forked this project, made the change, and submitted a pull request. My team has an Ionic Angular app which references the forked project in package.json.

If you or your team would like to continue referencing my fork, I do not intend on ever updating it, so the code will remain the same if/until the pull request is merged.

@jcesarmobile
Copy link
Member

I've just merged #580, which fixes the same issue, closing

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.

4 participants