-
Notifications
You must be signed in to change notification settings - Fork 31
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
[Incontext Insights] wrapper component and service #53
Conversation
Uses content added here opensearch-project/dashboards-assistant#53 to decorate the Discover page with the new component. Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #53 +/- ##
==========================================
- Coverage 95.63% 87.98% -7.66%
==========================================
Files 48 52 +4
Lines 1261 1415 +154
Branches 312 346 +34
==========================================
+ Hits 1206 1245 +39
- Misses 53 168 +115
Partials 2 2 ☔ View full report in Codecov by Sentry. |
Uses content added here opensearch-project/dashboards-assistant#53 to decorate the Discover page with the new component. Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
7ac5f80
to
44987de
Compare
Uses content added here opensearch-project/dashboards-assistant#53 to decorate the Discover page with the new component. Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
Uses content added here opensearch-project/dashboards-assistant#53 to decorate the Discover page with the new component. Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
Uses content added here opensearch-project/dashboards-assistant#53 Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
e764c5e
to
b915fc9
Compare
public/components/palantir/index.tsx
Outdated
anchorPosition="upCenter" | ||
display="block" | ||
> | ||
<EuiPopoverTitle>{children}</EuiPopoverTitle> |
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.
Q: Why render children
again in popover title?
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.
i can sanitize this as the HTML is still coming through but I believe it was to the original UX suggestion. but it has been a while.
cc: @opensearch-project/opensearch-ux
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.
I see, I was thinking the children
could be something else other than a simple text node, if that's the case, the popover may look a bit strange?
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.
yeah. have updated it. also i think there were mocks at one point that add visualizations so its prolly a little bit over kill with the updated mocks i have modify it a bit
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.
public/components/palantir/index.tsx
Outdated
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 thinking if that's a good idea to implement Palantir
as a generic component/service in OSD core? Instead of having core plugins or other plugins to depend on dashboards-assistant
which doesn't sound optimum to me. Also having it in dashboards-assistant
to serve a specific use case makes it hard to extend, as assistant might NOT be the only use case(Maybe a bit over thinking?).
Currently, the only part that makes Palantir
component "assistant-specific" is the EuiPopoverFooter
section which renders the action(s) to execute. I can move Palantir
to OSD core and let the plugins to register the footer actions, here are some quick dummy code:
interface ActionConfig {
type: string
render: (palantirInput: PalantirInput, onSubmit: () => void) => React.ReactNode
}
type ActionListener = (palantirInput: PalantirInput) => void
// PalantirService in core
class PalantirService {
actions = new Map<string, ActionConfig>
handlers = new Set<ActionListener>()
registerAction(actionConfig: ActionConfig) {
this.actions.set(actionConfig.type, actionConfig)
}
addActionHandler(listener: ActionListener) {
this.handlers.add(listener)
}
removeActionHandler(listener: ActionListener) {
this.handlers.delete(listener)
}
handlePalantir(palantirInput: PalantirInput) {
this.handlers.forEach(fn => fn(palantirInput))
}
}
Then the assistant plugin register an action
core.palantirService.registerAction({
type: 'assistant',
render: (palantirInput, onSubmit) =>
<>
<EuiText size="xs">Continue in chat</EuiText>
<EuiBadge
color="hollow"
iconType="chatRight"
iconSide="left"
onClick={onSubmit}
onClickAriaLabel="Click for suggestion"
>
{palantirInput.suggestion}
</EuiBadge>
</>
})
Move the current Palantir component to core and change the EuiPopoverFooter to:
<EuiPopoverFooter>
{palantirService.actions.get(targetAction)?.render(palantirInput, () => onSubmitClick(palantirInput))}
</EuiPopoverFooter>
And this is how Palantir will be used, targetAction
will be specified so that Palantir know what action to display
<Palantir targetAction="assistant" title="xx" input={input}><WhateverComponent /></Palantir>
And assistant plugin handle the action:
useEffect(() => {
const handleSuggestion = (event: { suggestion: string }) => {
// open chat window
setFlyoutVisible(true);
// start a new chat
props.assistantActions.loadChat();
// send message
props.assistantActions.send({
type: 'input',
contentType: 'text',
content: event.suggestion,
context: { appId },
});
};
core.palantirService.addActionHandler(handleSuggestion);
return () => {
core.palantirService.removeActionHandler(handleSuggestion);
};
}, []);
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.
i do like the way you think I was original planning on that path but thinking how far do we want to go.
there is a lot of existing services that are available within core OSD but are not used since they require proper wiring up.
i think eventual idea here would be to house this service and component within this plugin for the sake avoiding any breaking changes or issues within core OSD as this is still experimental. once enough feedback has been collected and the component is in its final state. then the core structure of this component can be placed into core OSD (and probably OUI). To which then this plugin then extends that core functionality of which you have mocked up and then if plugin developers decide that they want to hook up their features we can provide a guide but if they want to use a default functionality then they can just keep to using this plugin.
thoughts?
cc: @lezzago
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.
Totally understand your concern, I believe we do want the service we added to core to be stable and to serve more general use cases instead of specific one.
Just one question came in to my mind: if OSD core or other plugins which import
Palantir
component from dashboards-assistant
, will that break the build process when dashboards-assistant
is not in the plugins folder?
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.
Totally understand your concern, I believe we do want the service we added to core to be stable and to serve more general use cases instead of specific one.
Totally. It would make it easier in some ways. Also we give us time to create a provider for the React context so that externally we can call certain functions from the assistant.
Just one question came in to my mind: if OSD core or other plugins which import Palantir component from dashboards-assistant, will that break the build process when dashboards-assistant is not in the plugins folder?
Great question. So here's an example: https://github.com/opensearch-project/alerting-dashboards-plugin/pull/852/files there is a conditional there for actually using the component. and likely my fast follow will be handling the importing from a non existing file. probably an alias to trick the compiler
4e3dd85
to
d9b269c
Compare
d9b269c
to
27ee863
Compare
Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
0a3c135
to
bb72aff
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.
LGTM
* [Palantir] wrapper component and service Registry and component to be used by plugins. Example of usage: opensearch-project/alerting-dashboards-plugin#852 Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * recursion Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * more styling on component Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * one more try Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * Using wrapper component Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * almost Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * add arrow Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * cross Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * renamed Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * no destory Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * hacky styling Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * some rough tests and readme Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * add more doc Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * re-add type Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * fix tests Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * add i18n Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * enable target feature Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * remove invalid test now Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * missing palantir ref Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * make callable render function Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * update docs Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * cleaner incode styling Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * build config Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * move to server Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * not available for assets Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * fix path Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * check on call Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * dont give id Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * clean up styles one more time Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * add config to disable incontext not matter what Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * add doc Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * update changelog Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * more tests Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * address naming of classes Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * Add unused container Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * Removed default enabled config Signed-off-by: Kawika Avilla <kavilla414@gmail.com> --------- Signed-off-by: Kawika Avilla <kavilla414@gmail.com> (cherry picked from commit 5e4c971) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md
* [Palantir] wrapper component and service Registry and component to be used by plugins. Example of usage: opensearch-project/alerting-dashboards-plugin#852 Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * recursion Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * more styling on component Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * one more try Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * Using wrapper component Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * almost Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * add arrow Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * cross Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * renamed Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * no destory Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * hacky styling Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * some rough tests and readme Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * add more doc Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * re-add type Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * fix tests Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * add i18n Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * enable target feature Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * remove invalid test now Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * missing palantir ref Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * make callable render function Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * update docs Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * cleaner incode styling Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * build config Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * move to server Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * not available for assets Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * fix path Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * check on call Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * dont give id Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * clean up styles one more time Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * add config to disable incontext not matter what Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * add doc Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * update changelog Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * more tests Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * address naming of classes Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * Add unused container Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * Removed default enabled config Signed-off-by: Kawika Avilla <kavilla414@gmail.com> --------- Signed-off-by: Kawika Avilla <kavilla414@gmail.com> (cherry picked from commit 5e4c971) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md
* [Palantir] wrapper component and service Registry and component to be used by plugins. Example of usage: opensearch-project/alerting-dashboards-plugin#852 Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * recursion Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * more styling on component Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * one more try Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * Using wrapper component Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * almost Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * add arrow Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * cross Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * renamed Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * no destory Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * hacky styling Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * some rough tests and readme Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * add more doc Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * re-add type Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * fix tests Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * add i18n Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * enable target feature Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * remove invalid test now Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * missing palantir ref Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * make callable render function Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * update docs Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * cleaner incode styling Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * build config Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * move to server Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * not available for assets Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * fix path Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * check on call Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * dont give id Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * clean up styles one more time Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * add config to disable incontext not matter what Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * add doc Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * update changelog Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * more tests Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * address naming of classes Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * Add unused container Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * Removed default enabled config Signed-off-by: Kawika Avilla <kavilla414@gmail.com> --------- Signed-off-by: Kawika Avilla <kavilla414@gmail.com> (cherry picked from commit 5e4c971) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* [Palantir] wrapper component and service Registry and component to be used by plugins. Example of usage: opensearch-project/alerting-dashboards-plugin#852 Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * recursion Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * more styling on component Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * one more try Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * Using wrapper component Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * almost Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * add arrow Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * cross Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * renamed Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * no destory Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * hacky styling Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * some rough tests and readme Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * add more doc Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * re-add type Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * fix tests Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * add i18n Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * enable target feature Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * remove invalid test now Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * missing palantir ref Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * make callable render function Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * update docs Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * cleaner incode styling Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * build config Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * move to server Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * not available for assets Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * fix path Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * check on call Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * dont give id Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * clean up styles one more time Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * add config to disable incontext not matter what Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * add doc Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * update changelog Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * more tests Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * address naming of classes Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * Add unused container Signed-off-by: Kawika Avilla <kavilla414@gmail.com> * Removed default enabled config Signed-off-by: Kawika Avilla <kavilla414@gmail.com> --------- Signed-off-by: Kawika Avilla <kavilla414@gmail.com> (cherry picked from commit 5e4c971) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> # Conflicts: # CHANGELOG.md Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Description
Registry and component to be used by plugins.
issue resolves: #74
Example of usage:
opensearch-project/OpenSearch-Dashboards@25d7056
opensearch-project/alerting-dashboards-plugin#852
Issues Resolved
List any issues this PR will resolve, e.g. Closes [...].
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.