-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[engagement] Create Engagement plugin PoC #123339
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
Conversation
f420abb to
ea7ce1f
Compare
ea7ce1f to
2ccdd63
Compare
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.
Just passing by. The approach to have each plugin to manually opt-in by importing and using the component sounds good to me.
| <startServices.engagement.ContextProvider> | ||
| <Router history={history}> | ||
| <AgentPolicyContextProvider> | ||
| <PackageInstallProvider | ||
| notifications={startServices.notifications} | ||
| theme$={theme$} | ||
| > | ||
| <IntegrationsHeader {...{ setHeaderActionMenu, theme$ }} /> | ||
| {children} | ||
| <EngagementChat /> |
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.
Is this engagement.ContextProvider only used for the EngagementChat component? If so, did you though about exposing a context-wrapper component via the engagement plugin's contract instead of having to use both the plugin's contract to access the context provider and direct imports to import the chat component?
something like
<Router history={history}>
<....>
<startServices.engagement.Chat />
</...>
</Router>this is a pattern commonly used in other parts of Kibana when in need of exposing preconfigured/context-injected components to other plugins
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.
The idea was for this plugin to have more than one component, so the initial API matters.
That's an interesting approach! I'll have to think about it a bit more. My concerns with it at the moment are:
- getting
startServicesdown the tree in cases where a component is more granular, (e.g. a configured drop down control. - storing all
startServicesin context in the off chance you use a single component, whereas theContextProvidersimply provides services for components. I'm not sure if the difference is meaningful or not. - mocking the
startServicesin Storybook and Jest tests has been notoriously difficult. I'm loathe to introduce a complexstartServicesconstruct when you're only relying onContextProvider.
I need to look into this more.
src/plugins/engagement/public/components/iframe_chat/iframe_chat.tsx
Outdated
Show resolved
Hide resolved
| const getContext = () => { | ||
| const { location, navigator, innerHeight, innerWidth } = window; | ||
| const { hash, host, hostname, href, origin, pathname, port, protocol, search } = location; | ||
| const { language, userAgent } = navigator; | ||
| const { title, referrer } = document; |
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.
What's the purpose of this function compared to using window and document directly? To have more control over what's we're effectively sending and avoid potentially leaking things when calling chatIframe.contentWindow.postMessage?
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.
Just understanding what we have to send to the frame. This is a refactored bit of another draft PR from the Journey team, and, since I don't have insight into the messaging API, splitting it out like this seemed like a good way to keep track of precisely what we need to send, (and for testing purposes).
This issue seemed solved, although I didn't get last update from Drift support. But the chat window is sizing correctly now |
c11c00f to
8e466fd
Compare
8e466fd to
fbbe312
Compare
| id: 'user-id', | ||
| email: '[email protected]', | ||
| } | ||
| } |
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.
We need to find a way to obtain a Kibana or Cloud user data here. Particularly we need to get user ID (might be Kibana user's username) and user email (this should be the same as in Cloud for Cloud users)
@pgayvallet maybe you know how to do this?
|
Closing in favor of #123772 |
Summary
This is a proof-of-concept for a new plugin I'm calling "Engagement", which enables the integration of Drift on certain pages in Kibana. It is fully mocked in Storybook, as well.
Rather than add the logic to every page, this approach opts for plugins to add the
engagementplugin as a dependency, wrap their app in theServicesContextprovided by thestartcontract, and explicitly render the chat component. I opted for this approach so we can be surgical in how we add this component, (for now).Why a new plugin?
cloudis anxpackplugin, which limits its code use in OSS plugins.To Test
Storybook
Run
yarn storybook engagementfrom/kibana.In Kibana
Add the following to
kibana.dev.yml:Known Issues
public/plugin.tsx. I confirmed it works on the server, though.pocneed to be sourced appropriately, rather than set in config.100pxwide after clicking on the chat bubble.Next steps