Adding optional prop for lazy load the intercom script - #236
Conversation
Fixing timeout prop
3d25fa1 to
bb29b5eComparef14b398 to
97186f9CompareFix spelling Fix linting
733c31e to
4b353ecCompareThanks for the effort that you've put into this. I'll wait for your response on #237. But I'm not really eager to change this initialize snippet since it's copied from the official Intercom SPA manual. I will also have to take a look at the side effects of this |
Siggnja
commented
May 1, 2021
@devrnt Yes I noticed the failing test, I didn't want to put in more effort until I got some feedback from you. If this is something you would be willing to add to the codebase I would of course make sure this is a progressive enhancement and that it won't affect current functionality. I could even look into adding a test that covers this new functionality. |
…port' of github.com:Siggnja/react-use-intercom into pr/Siggnja/236-1
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
devrnt
commented
May 2, 2021
@Siggnja Looking good, I would like to see a small section in the |
Siggnja
commented
May 4, 2021
@devrnt Thanks for the comments, I've got some time tomorrow to update the PR and maybe add some tests. |
79d48bc to
d9c382dCompareSiggnja
commented
May 4, 2021
@devrnt I've added a test, let me know if you were thinking about something different. One thing I thought about while implementing this was whether we should support loading the intercom script before the delay if the user clicks/hovers over a custom launcher before the timeout has ended. This is my particular use case, we've got our own launcher which triggers the .show() action when clicked. This would mean that if users click the launcher before the delay they wouldn't experience a relatively any input delay. This is of course a special case which users would rarely encounter. |
…use-intercom into pr/Siggnja/236-1
There was a problem hiding this comment.
First of all, thank you for all the effort, really appreciate it!
@devrnt I've added a test, let me know if you were thinking about something different.
I'll check these asap
One thing I thought about while implementing this was whether we should support loading the intercom script before the delay if the user clicks/hovers over a custom launcher before the timeout has ended. This is my particular use case, we've got our own launcher which triggers the .show() action when clicked. This would mean that if users click the launcher before the delay they wouldn't experience a relatively any input delay. This is of course a special case which users would rarely encounter.
I've been thinking the same, however introducing a loading state seems too complex, especially since Intercom wants to avoid this themself. I declare myself: there is no way to check the "has initialized state" with the vanilla snippet, invoking methods before the snippet is initialized just doesn't do anything.
However, something that could be done is keeping track of the isInitialized state. Again maybe this introduces a bit too much complexity for users which is sufficient with a simple implementation. Also, it will probably mean that we'll have to re-render the whole thing, maybe a dealbreaker since at the moment it never rerenders, however, the argument the above argument carries more weight imo
Uh oh!
There was an error while loading. Please reload this page.
Alright, the test seems good to me. I've added a section in the Just one small change left and we're good to go 🚀 |
1ecf6f7 to
1ddadcdCompareSiggnja
commented
May 9, 2021
@devrnt Addressed your comment and removed some unused stuff from the example. Please take a look and let me know if there is anything else you need from me. |
devrnt
commented
May 10, 2021
@Siggnja Thanks, looking very good! I'll merge and release it soon! |
devrnt
commented
May 10, 2021
Released in v1.2.0 |
Solves #237