nventive / Binding.Intercom

Xamarin Binding for Intercom
Other
12 stars 6 forks source link

Supporting Intercom SDK V 6.1.0 #12

Closed HamzaRabah closed 4 years ago

HamzaRabah commented 4 years ago

Hi @jeremiethibeault, @jeremienventive , @MatFillion Please have a look and let me know your feedback. Thanks. Resolves #10
Resolves #9

jeremiethibeault commented 4 years ago

Hey @HamzaRabah, thanks for the PR.

I'm curious, how does it compare to the implementation described in issue #10

https://github.com/guillermo-gerard/Intercom.android.xamarin.binding

HamzaRabah commented 4 years ago

Actually, I didn't notice nor have a look at this implementation, but when you mentioned it, I found that this PR contains the issue with uploading gif/image. I am open to any solution or suggestion in order to upgrade this package to V6.1.0 as fast as possible and to keep the implementation in a single place, What do you think? @guillermo-gerard @jeremiethibeault

jeremiethibeault commented 4 years ago

I think we all agree that having a single place for the binding is the best approach :)

Can you take a look at his implementation and see what the differences are? I think I saw that the android transformations were different, we should probably update ours.

guillermo-gerard commented 4 years ago

Hi guys, as I've mentioned in issue #10 the solution I've found for the gif/image issue is to use that weird unversioned aar from the maven repository. The guys @Intercom still have the wrong file in their repo, they did not change it even when I suggest them to do so, so it appears that we are on our own here. @HamzaRabah What I meant is that you cannot fix the issue yourself in the binding, the problem occurs at runtime because there is a method missing in a class in the file "android-composer-gallery-2.0.2.aar", published for version 6.1.0 and located here (which is the file you probably use in your binding): https://github.com/intercom/intercom-android/tree/master/aar

HamzaRabah commented 4 years ago

Hi @guillermo-gerard Thank you for your helpful information, I've updated the binding and replaced this library android-composer-gallery-2.0.2.aar with the one you mentioned it android-composer-gallery-unspecified.aar, I've compared the transformations that you used with the ones in this binding, I couldn't find anything that needs to be updated here, What do you suggest?

jeremiethibeault commented 4 years ago

Hey @HamzaRabah, I looked at the transform files on Android and they seem different.

For example, this is the Metadata.xml for the base .aar vs this one in @guillermo-gerard's repo.

The one in this repository might be outdated, we should probably take a look at that, what do you think?

guillermo-gerard commented 4 years ago

I'm always prone to keep the metadata file (inside transforms folder) as small as possible. i.e.: I don't like to change anything if the binding project is working as it is by default. The metadata in my repository follows that "principle": I've added the minimum possible to get the binding working. So I guess that your PR should work if you use my metadata instead of what's in this repo (disclaimer: I didn't take a deep look at this repo's transforms metadata) But....as always, there is a "but...": Keep in mind that I've only tested my binding in Android because I'm using Xamarin for Android with native views. I don't use Forms, and I didn't test anything in iOS (because, in my particular case, I don't need to do it) so it could be a great idea to give this a test for those cases if you use the metadata from my implementation. Hope this helps

HamzaRabah commented 4 years ago

Hi, Actually, I am still using the metadata files from this repo, because they were changing the namespace/function/argument names and I don't want to introduce breaking changes in this version, I have tested this binding with Xamarin forms on Android and I couldn't find any issue, I couldn't test on iOS nowadays.

HamzaRabah commented 4 years ago

Hi @jeremiethibeault Any update?

jeremiethibeault commented 4 years ago

Hey @HamzaRabah, we didn't get a chance yet to test this PR (Android & iOS) but we'll do it really soon as we also need to update to version 6.

Thanks!

azure-pipelines[bot] commented 4 years ago
No pipelines are associated with this pull request.
MatFillion commented 4 years ago

/azp run

azure-pipelines[bot] commented 4 years ago
Azure Pipelines successfully started running 1 pipeline(s).