Hubspot #281
Hubspot #281
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
|
@drwlrsn when you have some time, would you ming signing that CLA (it just asks clicking a checkbox) to be able to merge this? Thank you 🙏 |
|
Having a look now to update this with an example and updating the readme as per the lastest changes on the repo |
Just added the example and the readme instructions. This is how it looks locally: |
| const Page: NextPage = () => ( | ||
| <LiveChatLoaderProvider | ||
| provider="hubSpot" | ||
| providerKey="" |
There was a problem hiding this comment.
This will need a correct value to work, but I haven't found a public one that I could share
|
Hey @drwlrsn 👋, when you have some time (if at all possible), would you be ok to sign https://github.com/calibreapp/react-live-chat-loader/pull/281#issuecomment-1590928909🙏 @benschwarz and @thefoxis when you have some time, do you have any feedback for this PR? 😀 |
|
@jucallej Thank you for this work 🙌 @robmorieson will complete a code review. We'll have to figure something out for the Hubspot demo key, at the moment I'm not sure how to handle that! |
There was a problem hiding this comment.
@jucallej Thanks for submitting this PR 🙌 The code itself looks solid, I've just left a few review comments/suggestions.
In regards to the implementation itself, I did run into some runtime errors while testing on mobile (iOS 16.4 simulator) so that might require some investigating:
src/components/HubSpot/index.tsx
Outdated
| </g> | ||
| </g> | ||
| </g> | ||
| </svg> |
There was a problem hiding this comment.
Looks like this svg might benefit from being run through SVGO for optimisation. There's also a handy online tool for this: https://jakearchibald.github.io/svgomg/
There was a problem hiding this comment.
Great catch! I've just updated it in the last commit 😀
src/components/HubSpot/index.tsx
Outdated
| backgroundColor?: string | ||
| loader?: ReactElement | ||
| }): JSX.Element | null => { | ||
| const [state, loadChat] = useChat({ loadWhenIdle: false }) |
There was a problem hiding this comment.
Generally we would want loadWhenIdle to be true, unless there is a specific reason HubSpot needs this set to false?
There was a problem hiding this comment.
Sounds good! I've just changed that in the last commit 😀
src/providers/hubSpot.ts
Outdated
| window.HubSpotConversations.widget.status().loaded | ||
| ) | ||
| }, | ||
| // Allow intercom to complete loading before removing fake widget |
There was a problem hiding this comment.
Just a minor typo in the comment here ✍️
There was a problem hiding this comment.
Great catch! I've just updated it in the last commit 😀
src/providers/hubSpot.ts
Outdated
| }, | ||
| // Allow intercom to complete loading before removing fake widget | ||
| () => { | ||
| console.debug({ |
There was a problem hiding this comment.
We can probably strip out the console.debug instances prior to merge
There was a problem hiding this comment.
Great catch! I've just updated it in the last commit 😀
Thanks for the review 🙌! I've refactored it to use a method called isHubspotWidgetDefined and use it throughout, and seems to work at least on safari on mac. Is it still failing with the latest changes? What emulator are you using to test? 😀 |
Co-authored-by: Christopher Van <203725+cvan@users.noreply.github.com>
This is using the work of @drwlrsn #130 but I moved it into my own fork, as I didn't have permissions to push to your repo @drwlrsn hope that's ok. Thanks for the work 🙏
What does this PR introduce?
In a few bullet points, please describe the changes this Pull Request makes. e.g.:
Related issues
Are there any related issues or feature requests this work will resolve? Please mention them here. e.g.:
Screenshots