-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
[cloud] Create IFRAME chat component; add to Unified Integrations #123772
Conversation
Pinging @elastic/fleet (Team:Fleet) |
4af1be1
to
75ba747
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.
First pass; added a handful of comments with some ideas on how to simplify. I'm admittedly not a storybook expert, so I focused mainly on the rest of the changes to cloud
.
Still needs tests, but overall I think it makes sense so far.
import { defaultConfig } from '@kbn/storybook'; | ||
|
||
module.exports = defaultConfig; |
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.
nit
import { defaultConfig } from '@kbn/storybook'; | |
module.exports = defaultConfig; | |
export { defaultConfig } from '@kbn/storybook'; |
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.
FFR: Storybook crashes if you do this.
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.
really? interesting
Co-authored-by: Luke Elmers <lukeelmers@gmail.com>
@elasticmachine merge upstream |
ed8da60
to
2926d14
Compare
2926d14
to
c5a5059
Compare
@clintandrewhall I can confirm that Drift already will have all the user data it needs to control the chat display logic by the time a user visits Kibana. We assume that a user visits Kibana after they visit Cloud UI and a user in Kibana has the same email as a user in Cloud UI. In that case we already have sent all the relevant data over to Drift (trial status, domain, etc). And at the moment a user visits Kibana we send a user's email to a Drift as a part of At present the chat icon always appears because there is a playbook in Drift matching a visitor context (see screenshot). If we remove that playbook, then no playbook is matched, then no chat icon displayed in Kibana. |
@pgayvallet I started with its own plugin in #123339. 🤷 It's going to be placed in a few plugins, yes. I think if we hit a circular dependency, we'll have to extract it out. I'll take a look when I'm back at my desk, see if we have a conflict. Thank you so much for your review, as well!! |
I had advocated for keeping this centralized in the Cloud plugin until we have an agreed-upon long term plan (Clint is cooking up an RFC for that). My reasoning was mainly that we could have 3rd party code -- Fullstory & Drift -- living in one place in Kibana, which would make it very clear to on-prem users where cloud-specific code lives, and give us some assurance that this code would never run unless explicitly configured in a cloud deployment. But I agree that eventually we'll need a generalized solution for handling 3rd party code in Kibana, and due to the existing cloud plugin dependencies, this may need to be its own plugin. |
@elasticmachine merge upstream |
Keeping it in the cloud plugin until we encounter such cyclic dependencies issues is fine with me, I just wanted to point this out in case it was overlooked in the design decision. |
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
async chunk count
ESLint disabled line counts
References to deprecated APIs
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
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.
Overall I am comfortable with where we ended up on this. Thanks for all your patience @clintandrewhall ❤️
Could we wait to merge until Monday afternoon (NASA time) so that @pgayvallet has an opportunity to provide any remaining feedback he may have?
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.
LGTM. Thanks for the observable changes
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.
ResponseOps changes LGTM!
Summary
This PR creates the Chat component for Cloud and adds it to Unified Integrations.
Testing
You can test the component in Storybook:
/kibana yarn storybook cloud
You can also test locally by adding the following to
kibana.dev.yml
:Integration tests can be run by adding
CHAT_URL
andCHAT_IDENTITY_SECRET
to both the functional test server and runner: