-
Notifications
You must be signed in to change notification settings - Fork 58
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
Update Intercom component to accept style-related props #101
Update Intercom component to accept style-related props #101
Conversation
The supported props are the same as what intercom supports as messenger attributes.
Intercoms attributes are snake_case but the props to the component are camelCase to remain inline with the rest of the project.
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/calibreapp/react-live-chat-loader/WJiRbhiEwUs2bwHuv5ibwc4yJhcX |
src/components/Intercom/index.tsx
Outdated
@@ -154,7 +170,10 @@ const Intercom = ({ color }: Props): JSX.Element | null => { | |||
} | |||
|
|||
Intercom.defaultProps = { | |||
color: '#333333' | |||
color: '#333333', | |||
alignment: 'left', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be right
to maintain existing implementations?
@myleslinder I found this in the Intercom Docs:
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 |
All props provided to the Intercom component are deemed to be the source of truth for styling those aspects of the widget. This ensures there's no style mismatch between the facade and the loaded widget. A styling mismatch can still occur if a custom icon is set in Intercom. To mirror the default Intercom widget behavior padding props are only applied on screen sizes with a width greater than 900px. For smaller screen sizes the default padding of 20px is used. Fix incorrect alignment proptypes default prop value
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 Let me know your thoughts on the changes. |
@myleslinder We're getting an issue building the Intercom component due to
|
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? |
@myleslinder Build is now passing 👍 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: Therefore as it is now, we'll have mismatched positions on browser sizes < 900px as it's loading: |
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? |
Hey @myleslinder, we'd love to merge these improvements you've made. Did you get a chance to review the feedback? |
Hey, really sorry this slipped through the cracks. I'll get this over the line this week. |
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.:
alignment
,verticalPadding
, andhorizontalPadding
propsREADME.md
with a better description for contributionsRelated issues