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

[WIP] HelpScout enhancements #830

Closed
wants to merge 5 commits into from
Closed

[WIP] HelpScout enhancements #830

wants to merge 5 commits into from

Conversation

delfipolito
Copy link
Contributor

@delfipolito delfipolito commented Jun 7, 2019

Fixes #782.

@luisivan luisivan mentioned this pull request Jun 7, 2019
18 tasks
@luisivan luisivan requested review from AquiGorka and removed request for AquiGorka June 10, 2019 15:10
script.innerHTML = BEACON_EMBED
document.body.appendChild(script)
}
window.Beacon('init', HELPSCOUT_ID)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super how this works!
I think this 2 actions (init and setBeaconInit) should be triggered in the onload method from script no?
Maybe attach the script to the head instead to the body?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On another note, by removing the beaconReady prop the loader is no longer shown while the script is loading, you can test this by throttling the network speed in chrome, let's figure a way to show the LoadingRing to avoid the modal from disappearing and reappearing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this 2 actions (init and setBeaconInit) should be triggered in the onload method from script no?

Beacon.init() gets queued and it’s probably a good idea to keep it first, as the script will probably refuse to execute the other methods if they are called before.

beaconInit is used to know that the script is being initialized, so that we don’t inject the script twice.

</Helmet>
const beacon = useCallback(
(...params) => {
if (window.Beacon && optedIn && beaconInit) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens when this function is called but window.Beacon does not exist is the calls gets lost, should we be queueing them? or at least sending them to the future via a setTimeout?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to check again, but IIRC this window.Beacon in the condition is only here as a safety measure because we don’t control what the HelpScout script does with the object.

The window.Beacon object should exist as soon as optedIn changes to be true, and BEACON_EMBED is doing exactly this:

  1. Creating the window.Beacon function.
  2. Queuing calls to window.Beacon.
  3. Injecting the script (that uses the queue).

As a note, the reason why it there is a wrapping function here (instead of exposing window.Beacon directly) is because HelpScout replaces it in a totally unpredictable way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The window.Beacon object should exist as soon as optedIn

This is not in synch right? So, maybe there is some delay before the window.Beacon object exists.

@delfipolito delfipolito requested a review from bpierre June 11, 2019 12:16
@stale stale bot added the abandoned label Jul 13, 2019
@stale stale bot closed this Jul 20, 2019
@bpierre bpierre reopened this Jul 20, 2019
@stale stale bot removed the abandoned label Jul 20, 2019
@stale stale bot added the abandoned label Aug 19, 2019
@aragon aragon deleted a comment from stale bot Aug 19, 2019
@stale stale bot removed the abandoned label Aug 19, 2019
@stale stale bot added the abandoned label Sep 18, 2019
@john-light john-light changed the title [WIP] HelpScout enhacements [WIP] HelpScout enhancements Sep 18, 2019
@stale stale bot removed the abandoned label Sep 18, 2019
@aragon aragon deleted a comment from stale bot Oct 16, 2019
@aragon aragon deleted a comment from stale bot Oct 16, 2019
@stale stale bot added the abandoned label Nov 15, 2019
@aragon aragon deleted a comment from stale bot Nov 18, 2019
@stale stale bot removed the abandoned label Nov 18, 2019
@stale stale bot added the abandoned label Dec 18, 2019
@sohkai sohkai removed the abandoned label Dec 18, 2019
@aragon aragon deleted a comment from stale bot Dec 18, 2019
@stale stale bot added the abandoned label Jan 17, 2020
@aragon aragon deleted a comment from stale bot Jan 17, 2020
@stale stale bot removed the abandoned label Jan 17, 2020
@stale stale bot added the abandoned label Feb 16, 2020
@aragon aragon deleted a comment from stale bot Feb 17, 2020
@stale stale bot removed the abandoned label Feb 17, 2020
@stale stale bot added the abandoned label Mar 18, 2020
@aragon aragon deleted a comment from stale bot Mar 18, 2020
@stale stale bot removed the abandoned label Mar 18, 2020
@stale stale bot added the abandoned label Apr 17, 2020
@stale stale bot closed this Apr 24, 2020
@aragon aragon deleted a comment from stale bot Apr 24, 2020
@sohkai sohkai deleted the helpscout branch June 7, 2020 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HelpScout: optimize loading
4 participants