-
Notifications
You must be signed in to change notification settings - Fork 45
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
Adding optional prop for lazy load the intercom script #236
Conversation
Fixing timeout prop
3d25fa1
to
bb29b5e
Compare
f14b398
to
97186f9
Compare
Fix spelling Fix linting
733c31e
to
4b353ec
Compare
Thanks 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 |
@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
@Siggnja Looking good, I would like to see a small section in the |
@devrnt Thanks for the comments, I've got some time tomorrow to update the PR and maybe add some tests. |
Fix spelling Fix linting
Fix spelling
79d48bc
to
d9c382d
Compare
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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
1ddadcd
Compare
@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. |
@Siggnja Thanks, looking very good! I'll merge and release it soon! |
Released in v1.2.0 |
Solves #237