search

talkjs / talkjs-flutter

Flutter SDK for the TalkJS Chat API
https://talkjs.com
BSD 3-Clause "New" or "Revised" License
9 stars 13 forks source link

feat: Adds navigationDelegate for handling links #15

Closed Alexander-Ordina closed 1 year ago

Alexander-Ordina commented 1 year ago

Adds NavigationDelegate to the chatbox for handling links.

See https://github.com/talkjs/talkjs-flutter/issues/14

mrcnkoba commented 1 year ago

Thank you @Alexander-Ordina for your contribution to our plugin. We will review your PR and get it released.

eteeselink commented 1 year ago

Hi @Alexander-Ordina, first off, thanks for contributing this PR!

We're reviewing it, and while the code change is tiny, it may impact architecture changes we got lined up in the near future, so we'll need some time to evaluate it. (note that we'll also need a signed CLA, even for such tiny code changes unfortunately, as discussed in the other PR you made)

Can you confirm that for your own project, you're unblocked since you can use your forked version of the SDK for the time being, until we merge this PR?

Alexander-Ordina commented 1 year ago

Hi @eteeselink, I just got confirmation that the CLA is signed and send to your legal department. I'm guessing the architecture change you are talking about is the switch to InAppWebView, to get the same behavior with InAppWebView the shouldOverrideUrlLoading property can be used.

We do not plan to use the fork I made, the fork was only to make is possible to create a Pull Request, so until this is solved we are still blocked for rolling out Talkjs in our App.

eteeselink commented 1 year ago

Hi Alexander,

Fantastic! For our understanding: we recently open-sourced the flutter SDK specificallly to make interactions like these possible, so we're very happy with your contributions. Sorry about our delay in addressing them.

That said, another goal we had with open-sourcing the SDK was to unblock customers, ie not keep customers blocked on our development speed for things inside the SDK, if something is missing. I see that you're nevertheless unable to temporarily use your fork until we worked things out, which surprised me. Would you be able to shed some light on the dynamics on your side that prevent you from going live now using your fork, and switching over to our official SDK once we merged your PR? (or addressed the problem it solves in an alternative way)

bugnano commented 1 year ago

Hi @eteeselink, I just got confirmation that the CLA is signed and send to your legal department. I'm guessing the architecture change you are talking about is the switch to InAppWebView, to get the same behavior with InAppWebView the shouldOverrideUrlLoading property can be used.

You're totally right about the architecture change being the switch to InAppWebView. Thank you for pointing us at the shouldOverrideUrlLoading property, and as you can see its interface is slightly different than the NavigationDelegate. Given that we would like to be able to switch implementation any time we want, can you come up with a solution that exposes the properties that you need in a WebView-implementation independent way for this PR?

Alexander-Ordina commented 1 year ago

@bugnano I have added a onUrlNavigation function on the chatbox widget, this property can be used for adding the navigation logic independent of which webview is used. It is still needed to have a webview dependent implementation to translate this function for the type of webview used, for this I added the private function _shouldNavigateToUrl which is used as the NavigationDelegate for the flutter webview. When switching to the InAppWebview this function will have to be rewritten to work with it.

Alexander-Ordina commented 1 year ago

Hi @eteeselink, I understand that it might be surprising that we do not want to use the temporary fork. We want to prevent creating unnecessary forks which we have to maintain, we only do this if there is no other option. We try to make sure that we have plenty of time to solve issues with plugins before we release new features either by filing issues or creating pull requests.

We expect that these issues can probably be solved before we go live with chat feature in our app, to make sure that is the case we would like to know when there is a new release for TalksJs plugin planned?

zSoNz commented 1 year ago

@eteeselink , hey! Seems like in last version you forgot add this functionality :(

vickz84259 commented 1 year ago

Hey @zSoNz, I'm a developer at TalkJS. Regarding the feature above, it was an intentional decision to not include it. For versions 0.6.0 and lower of our SDK, links inside the chat would open inside the webview rather than in an external browser. This was the initial motivation for implementing navigationDelegate as highlighted in this issue. This behaviour has been changed since version 0.7.0.

Was there another use case you intended to use navigationDelegate for?