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 Messenger provider and component #11

Merged
merged 4 commits into from
Mar 20, 2020

Conversation

FateXRebirth
Copy link
Contributor

Description

  • Add a messenger provider
  • Add a messenger component
  • Adjust API of LiveChatLoaderContext
  • Adjust useChat hook
  • Update the documentation accordingly.

Related Issue

Related to issue #5

Motivation and Context

Since our project needs to use customer chat SDK for business purpose, but the version official provides make our website slow, so we want to create a proper provider and component using a mechanism like requestIdleCallback.

How Has This Been Tested?

Use storybook to test if it works properly

Screenshots (if appropriate):

Screen Shot 2020-02-25 at 11 10 50 AM

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

Copy link
Contributor

@mikedijkstra mikedijkstra left a comment

Choose a reason for hiding this comment

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

@FateXRebirth thanks so much for creating this PR!

I'm not very familiar with Messenger but I've had a look through the PR and it looks close, we just need to tidy a few things up and look into some additional customisation options.

You said you've tested this in a Storybook, do you have a developer page you'd be willing to add details for in an implementation in the Storybook file? If not, would you mind adding the implementation with dummy keys, then other developers can update the keys to test it out.

README.md Outdated Show resolved Hide resolved
src/hooks/useChat.js Outdated Show resolved Hide resolved
src/hooks/useChat.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
src/components/Messenger/index.js Outdated Show resolved Hide resolved
src/providers/messenger.js Outdated Show resolved Hide resolved
@FateXRebirth
Copy link
Contributor Author

@michaeldijkstra thanks so much for taking your time to review this PR and giving so many advices, I have adjusted some code and documentation, adding customization as well, please check it, thanks !

@mikedijkstra
Copy link
Contributor

mikedijkstra commented Mar 2, 2020

@FateXRebirth Thanks for updating. I've been testing this myself with a Facebook page and have noted a few things:

  1. The implementation details from Facebook only provide a page id and not an app id. Do you know if both are required or if we need to handle cases where you only have one or the other or both?

  2. I'm getting this error in the console: Refused to display 'https://www.facebook.com/…' in a frame because it set 'X-Frame-Options' to 'deny'. Have you seen this? Is there some config on the FB side I need to do?

  3. The code snippet I got has the version set to 6, does this need to be configurable or could we update to the latest version?

Snippet ```
<script> window.fbAsyncInit = function() { FB.init({ xfbml : true, version : 'v6.0' }); };
    (function(d, s, id) {
    var js, fjs = d.getElementsByTagName(s)[0];
    if (d.getElementById(id)) return;
    js = d.createElement(s); js.id = id;
    js.src = 'https://connect.facebook.net/en_US/sdk/xfbml.customerchat.js';
    fjs.parentNode.insertBefore(js, fjs);
  }(document, 'script', 'facebook-jssdk'));</script>

  <!-- Your customer chat code -->
  <div class="fb-customerchat"
    attribution=setup_tool
    page_id="100594861551567">
  </div>

</details>

@FateXRebirth
Copy link
Contributor Author

FateXRebirth commented Mar 2, 2020

@michaeldijkstra thanks for reminding, the following is the answer to your questions.

  1. APP_ID is no need if you only use customer chat. if you use other facebook feature like share and you also want to use this react-live-chat-loader, You have to set APP_ID as well, depends on scenario, I think we can adjust providerKey to PAGE_ID, and adjust APP_ID to optional props and update documentation in this case?

  2. Not a problem about config, this only happen when you use storybook to test this feature due to storybook use iframe to demostrate each use case, if you use other way to test, everything is OK.

  3. I tested it, I think use latest version is OK.

@mikedijkstra
Copy link
Contributor

@FateXRebirth Ok Great!

Let's update the providerKey to be the Page ID and make App ID optional as you suggested!

@FateXRebirth
Copy link
Contributor Author

@michaeldijkstra I have updated code and documentation, please inform me if you have other concern, thank you!

@mikedijkstra
Copy link
Contributor

@FateXRebirth I'm seeing the following error when loading messenger:

Uncaught TypeError: Cannot read property 'CustomerChat' of undefined

Do you see that too?

@FateXRebirth
Copy link
Contributor Author

FateXRebirth commented Mar 10, 2020

@michaeldijkstra it seems like something wrong in the preload phase, no any other informations? it shouldn't occur when everything is set up, Did you test it with your FB app and page? There is a config in FB app or page about whitelist host, if you test it in storybook, the host will be localhost, I am not sure if FB app or page support the localhost, I guess this may be your problem.

I have that problem as well, But I test it in our website, it works perfectly. There are some configurations about FB app or page that user must adjust it by themself, which we can't set up for them.

Copy link
Contributor

@mikedijkstra mikedijkstra left a comment

Choose a reason for hiding this comment

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

@FateXRebirth Okay, I got this working! I had to configure the domain in Facebook and turn my adblocker off 🤦‍♂

I've suggested two additional changes. I'm happy to refactor createCustomerChat into Messenger if you don't have time, just let me know

Thanks again for all your work on this!

greetingDialogDelay = '',
}) => {
if(!document.querySelector('.fb-customerchat')) {
const chat = window.document.createElement('div');
Copy link
Contributor

Choose a reason for hiding this comment

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

Given this is a react plugin, and we've got the Messenger component, what do you think about moving this to be output to JSX inside Messenger.js?

This would involve creating a new hook to get the required props but I think it'd be a cleaner implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please do it, your suggestion is great, I think you finish this would be better than me.

}

const open = () => {
window.FB.CustomerChat.show(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
window.FB.CustomerChat.show(false);
window.FB.CustomerChat.show(true);

I believe this should be true so that when a user clicks on the placeholder before it has loaded, we expand the chat widget once it has loaded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, please help me adjust this, thanks 👍

@FateXRebirth
Copy link
Contributor Author

@michaeldijkstra Just an mention, I found that http://127.0.0.1 is a valid domain for configuration of Facebook, But still got Refused to display 'https://www.facebook.com/…' in a frame because it set 'X-Frame-Options' to 'deny'. problem, Perhaps you could add some memos in documentation about the domain issue of customer chat plugin for those who using messenger provider

Thanks again for all your helps!

@calibreapp calibreapp deleted a comment from cla-bot bot Mar 11, 2020
@calibreapp calibreapp deleted a comment from cla-bot bot Mar 11, 2020
@mikedijkstra mikedijkstra merged commit 271618b into calibreapp:master Mar 20, 2020
@mikedijkstra mikedijkstra mentioned this pull request Mar 20, 2020
@mikedijkstra
Copy link
Contributor

@all-contributors please add @FateXRebirth for code

@allcontributors
Copy link
Contributor

@michaeldijkstra

I've put up a pull request to add @FateXRebirth! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants