react-live-chat-loader icon indicating copy to clipboard operation
react-live-chat-loader copied to clipboard

Update Intercom component to accept style-related props

Open myleslinder opened this issue 4 years ago • 9 comments

The supported props are the same as what intercom supports as messenger attributes.


What does this PR introduce?

In a few bullet points, please describe the changes this Pull Request makes. e.g.:

  • Updates the Intercom component to accept alignment, verticalPadding, and horizontalPadding props
  • Resolves feature request #92
  • updates README.md with a better description for contributions

Related issues

  • Resolves feature request #92

myleslinder avatar Sep 05 '21 02:09 myleslinder

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/calibreapp/react-live-chat-loader/WJiRbhiEwUs2bwHuv5ibwc4yJhcX
✅ Preview: https://react-live-chat-loader-git-fork-myleslinder-f-a88a56-calibreapp.vercel.app

vercel[bot] avatar Sep 05 '21 02:09 vercel[bot]

@myleslinder I found this in the Intercom Docs:

Messenger will revert to the default padding setting (20 / 20) on mobile

Therefore we need to ensure that on mobile the padding values are set to 20 and only set them to those specified by the props after the breakpoint.

Also, these values need to be added into window.intercomSettings to apply to the actual widget. We could leave this up to the implementor and just make a note in the README?

mikedijkstra avatar Sep 07 '21 03:09 mikedijkstra

I've addressed the issue with the incorrect alignment value being set on default props for proptypes.

I've also added the changes for the mobile padding and forwarding the provided props to window.intercomSettings. Re: intercom settings, I've forwarded all the provided props, as opposed to just the ones for padding, so that it's consistent that the values you provide to the Intercom component are all the source of truth for the facade and the loaded widget.

Let me know your thoughts on the changes.

myleslinder avatar Sep 07 '21 23:09 myleslinder

@myleslinder We're getting an issue building the Intercom component due to window.intercomSettings ?? {}. You probably can't see the logs so I'll paste them here:

09:37:50.930 | Failed to compile.
-- | --
09:37:50.938 | ../module/components/Intercom/index.js 108:87
09:37:50.938 | Module parse failed: Unexpected token (108:87)
09:37:50.939 | You may need an appropriate loader to handle this file type, currently no loaders are configured to process this file. See https://webpack.js.org/concepts#loaders
09:37:50.939 | \|   const windowWidth = useWindowWidth();
09:37:50.939 | \|   useEffect(() => {
09:37:50.939 | >     window.intercomSettings = _objectSpread(_objectSpread({}, window.intercomSettings ?? {}), {}, {
09:37:50.939 | \|       alignment,
09:37:50.939 | \|       background_color: color,
09:37:50.940 | > Build error occurred
09:37:50.943 | Error: > Build failed because of webpack errors
09:37:50.943 | at build (/vercel/path0/website/node_modules/next/dist/build/index.js:13:900)
09:37:50.961 | npm ERR! code ELIFECYCLE
09:37:50.961 | npm ERR! errno 1
09:37:50.965 | npm ERR! [email protected] build: `next build`
09:37:50.966 | npm ERR! Exit status 1
09:37:50.966 | npm ERR!
09:37:50.966 | npm ERR! Failed at the [email protected] build script.
09:37:50.966 | npm ERR! This is probably not a problem with npm. There is likely additional logging output above.
09:37:50.971 | npm ERR! A complete log of this run can be found in:
09:37:50.972 | npm ERR!     /vercel/.npm/_logs/2021-09-07T23_37_50_966Z-debug.log
09:37:50.987 | Error: Command "cd .. && npm run build && npm link website/node_modules/react && cd website && npm run build" exited with 1

mikedijkstra avatar Sep 08 '21 01:09 mikedijkstra

Thanks so much for getting this over the line @myleslinder.

I'm happy with docs on review, is there anything else outstanding for you @mikedijkstra?

benschwarz avatar Sep 13 '21 22:09 benschwarz

@myleslinder Build is now passing :+1:

I tested the responsive behaviour locally and it seems Intercom isn't using the browser width alone. When I emulate a mobile device the real widget is locked at 20px and 20px, however when I resize the browser it's at the position set by the updated settings:

Screen Shot 2021-09-14 at 11 13 05 am Screen Shot 2021-09-14 at 11 13 20 am

Therefore as it is now, we'll have mismatched positions on browser sizes < 900px as it's loading:

Screen Shot 2021-09-14 at 11 19 11 am

mikedijkstra avatar Sep 14 '21 01:09 mikedijkstra

Hi @myleslinder — Were you able to replicate the issue I saw above? Are you still interested in getting this wrapped up and over the line?

mikedijkstra avatar Jan 06 '22 23:01 mikedijkstra

Hey @myleslinder, we'd love to merge these improvements you've made. Did you get a chance to review the feedback?

benschwarz avatar May 04 '22 04:05 benschwarz

Hey, really sorry this slipped through the cracks. I'll get this over the line this week.

myleslinder avatar May 04 '22 15:05 myleslinder