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

Adding optional prop for lazy load the intercom script #236

Merged
merged 16 commits into from
May 10, 2021

Conversation

Siggnja
Copy link
Contributor

@Siggnja Siggnja commented Apr 30, 2021

Solves #237

@Siggnja Siggnja force-pushed the implement-timeout-support branch from 3d25fa1 to bb29b5e Compare April 30, 2021 12:15
@Siggnja Siggnja force-pushed the implement-timeout-support branch from f14b398 to 97186f9 Compare April 30, 2021 12:20
@Siggnja Siggnja force-pushed the implement-timeout-support branch from 733c31e to 4b353ec Compare April 30, 2021 12:46
@devrnt
Copy link
Owner

devrnt commented May 1, 2021

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 setTimeout even with a 0ms timeout it will put it on the event loop

@Siggnja
Copy link
Contributor Author

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
src/provider.tsx Outdated Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
@devrnt
Copy link
Owner

devrnt commented May 2, 2021

@Siggnja Looking good, I would like to see a small section in the README that "explains" this functionality and also update the API table

@Siggnja
Copy link
Contributor Author

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.

@Siggnja Siggnja force-pushed the implement-timeout-support branch from 79d48bc to d9c382d Compare May 4, 2021 22:42
@Siggnja
Copy link
Contributor Author

Siggnja 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.

Copy link
Owner

@devrnt devrnt left a 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

playground/modules/useIntercom/useIntercomWithDelay.tsx Outdated Show resolved Hide resolved
@devrnt
Copy link
Owner

devrnt commented May 5, 2021

Alright, the test seems good to me. I've added a section in the README about this initialization.

Just one small change left and we're good to go 🚀

@Siggnja Siggnja force-pushed the implement-timeout-support branch from 1ecf6f7 to 1ddadcd Compare May 9, 2021 17:49
@Siggnja
Copy link
Contributor Author

Siggnja 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.

@Siggnja Siggnja requested a review from devrnt May 10, 2021 09:57
@devrnt
Copy link
Owner

devrnt commented May 10, 2021

@Siggnja Thanks, looking very good! I'll merge and release it soon!

@devrnt devrnt merged commit 5327e3c into devrnt:master May 10, 2021
@devrnt
Copy link
Owner

devrnt commented May 10, 2021

Released in v1.2.0

@Siggnja Siggnja deleted the implement-timeout-support branch May 14, 2021 20:42
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.

2 participants