-
Notifications
You must be signed in to change notification settings - Fork 8.5k
Add export derivative integration type #223994
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
Add export derivative integration type #223994
Conversation
|
@elasticmachine merge upstream |
This comment was marked as outdated.
This comment was marked as outdated.
15ca9eb to
7afcdbe
Compare
This comment was marked as duplicate.
This comment was marked as duplicate.
7afcdbe to
be2c36e
Compare
This comment was marked as duplicate.
This comment was marked as duplicate.
be2c36e to
e8807fc
Compare
This comment was marked as duplicate.
This comment was marked as duplicate.
e8807fc to
5a7fe2d
Compare
This comment was marked as duplicate.
This comment was marked as duplicate.
5a7fe2d to
86da0f8
Compare
This comment was marked as duplicate.
This comment was marked as duplicate.
86da0f8 to
0e05f50
Compare
This comment was marked as duplicate.
This comment was marked as duplicate.
1 similar comment
|
/ci |
|
Pinging @elastic/appex-sharedux (Team:SharedUX) |
2ba6236 to
75b6097
Compare
| extends ShareIntegration<{ | ||
| label: ReactElement; | ||
| toolTipContent?: ReactNode; | ||
| flyoutContent: React.FC<{ closeFlyout: () => void }>; |
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 there any good reason we need to manage the flyout ourselves?
Just a thought: maybe it would be simpler for us to allow rendering whatever they want, and when the user clicks, they can open a flyout, a modal, or take some action right away.
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.
imo, If keeping something like this, then I'd suggest the api using render props. Imo clearer to use (explicit how label becomes clickable) and easier for us to implement (no cloneElement, for example)
eport interface ExportShareDerivatives
extends ShareIntegration<{
label: (props: {openFlyout}) => ReactNode
toolTipContent?: ReactNode;
flyoutContent: (props: { closeFlyout}) => ReactNode
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 there any good reason we need to manage the flyout ourselves? Just a thought: maybe it would be simpler for us to allow rendering whatever they want, and when the user clicks, they can open a flyout, a modal, or take some action right away.
It's really part of the desire to align what the export experience is, so it's predictable in a way
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.
imo, If keeping something like this, then I'd suggest the api using render props. Imo clearer to use (explicit how label becomes clickable) and easier for us to implement (no cloneElement, for example)
eport interface ExportShareDerivatives extends ShareIntegration<{ label: (props: {openFlyout}) => ReactNode toolTipContent?: ReactNode; flyoutContent: (props: { closeFlyout}) => ReactNode
I think this is a fair point, I like the modification you proposed definitely hands-off even more control. Thanks Anton!
|
I also thought that a derivative is an unusual name, have you considered |
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.
Pull Request Overview
This PR adds a new export derivative integration type, expanding the share integration functionality and adjusting several context and component implementations to support this new variant.
- Introduces the ExportShareDerivatives interface and related types.
- Updates UI components to support both export and export derivative integrations.
- Refactors context hook usage across components and tests.
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| x-pack/test/functional/page_objects/lens_page.ts | Updates test to pass a callback to clickPopoverItem. |
| src/platform/test/functional/page_objects/export_page.ts | Modifies clickPopoverItem to accept an optional opener callback. |
| src/platform/plugins/shared/share/public/types.ts | Adds the ExportShareDerivatives interface and updates type unions. |
| src/platform/plugins/shared/share/public/index.ts | Exports the new ExportShareDerivatives type. |
| src/platform/plugins/shared/share/public/components/tabs/link/index.tsx | Renames context hook usage for clarity. |
| src/platform/plugins/shared/share/public/components/tabs/embed/index.tsx | Renames context hook usage for clarity. |
| src/platform/plugins/shared/share/public/components/share_tabs.tsx | Replaces ShareMenuProvider with ShareProvider. |
| src/platform/plugins/shared/share/public/components/share_tabs.test.tsx | Updates tests to reflect the new context provider naming. |
| src/platform/plugins/shared/share/public/components/export_popover/export_popover.tsx | Refactors export popover implementation to include support for export derivatives. |
| src/platform/plugins/shared/share/public/components/context/index.tsx | Renames context and updates filtering logic in the hook. |
| src/platform/plugins/shared/share/public/components/context/index.test.tsx | Updates tests to use the revised context hook. |
| src/platform/packages/private/kbn-reporting/public/share/share_context_menu/register_csv_modal_reporting.tsx | Adds an icon property for CSV export integration. |
Comments suppressed due to low confidence (2)
x-pack/test/functional/page_objects/lens_page.ts:2010
- Verify that passing the clickExportButton callback as a parameter to clickPopoverItem is consistent with its usage elsewhere and meets the intended testing behavior.
await exports.clickPopoverItem('CSV', this.clickExportButton.bind(this));
src/platform/plugins/shared/share/public/components/context/index.tsx:58
- Review the filtering logic here: using (groupId ?? item?.groupId) means that when groupId is undefined, the condition always holds true. Confirm that this behavior is intentional.
(item) => item.shareType === shareType && item?.groupId === (groupId ?? item?.groupId)
| </EuiFlexItem> | ||
| </EuiFlexGroup> | ||
| </EuiFlyoutFooter> | ||
| {selectedMenuItemMeta!.group === 'export' ? ( |
Copilot
AI
Jun 16, 2025
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.
Consider explicitly verifying that selectedMenuItemMeta is defined before accessing its properties, rather than using a non-null assertion, to safeguard against potential runtime errors.
| {selectedMenuItemMeta!.group === 'export' ? ( | |
| {selectedMenuItemMeta && selectedMenuItemMeta.group === 'export' ? ( |
I'd chosen derivative to somewhat denote for this specific integration that it requires at least one active export integration for it be surfaced and especially that it's a UI construct rather it being a implicit implementation detail. I worry |
afeee98 to
09e88b5
Compare
09e88b5 to
720f86b
Compare
umbopepato
left a comment
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.
Thanks @eokoneyo for implementing this new extension point so quickly!
LGTM (considering the popovers z-index issue solved with EUI's recommendation)
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
Page load bundle
History
cc @eokoneyo |
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
This PR adds a new share integration type (i.e. the `exportDerivative`).
This integration is a special one that is only to be used in a scenario
where a team would like to offer some extra functionality that depends
on the availability of some `export` integration. Specifically because
of the way this has been coupled to be presented to the user the
`exportDerivative` type integration would only be displayed to the user
when at least one export integration exists.
This particular integration is also much more flexible, in that it only
accepts a label to denote what should be rendered to the user, and a
freeform component that will be rendered into the flyout that will in
turn open when the provided label is clicked.
One might configure an integration for this like so;
```ts
share.registerShareIntegration<ExportShareDerivatives>({
id: 'scheduledReporting',
groupId: 'exportDerivatives',
config: () => ({
label: ({ openFlyout }) =>
React.createElement(
EuiButton,
{ iconType: 'calendar', onClick: openFlyout },
'schedule report'
),
flyoutContent: ({ closeFlyout , flyoutRef }) => {
return React.createElement(
EuiButton,
{
iconType: 'calendar',
},
'Click me'
);
},
}),
});
```
The following config would produce the following experience;

<!--
### Checklist
Check the PR satisfies following conditions.
Reviewers should verify this PR satisfies this list as well.
- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [ ] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [ ] This was checked for breaking HTTP API changes, and any breaking
changes have been approved by the breaking-change committee. The
`release_note:breaking` label should be applied in these situations.
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [ ] The PR description includes the appropriate Release Notes section,
and the correct `release_note:*` label is applied per the
[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
### Identify risks
Does this PR introduce any risks? For example, consider risks like hard
to test bugs, performance regression, potential of data loss.
Describe the risk, its severity, and mitigation for each identified
risk. Invite stakeholders and evaluate how to proceed before merging.
- [ ] [See some risk
examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx)
- [ ] ...
-->
(cherry picked from commit b275c9e)
# Conflicts:
# src/platform/plugins/shared/share/public/components/export_popover/export_popover.tsx
# Backport This will backport the following commits from `main` to `8.19`: - [Add export derivative integration type (#223994)](#223994) <!--- Backport version: 10.0.1 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport) <!--BACKPORT [{"author":{"name":"Eyo O. Eyo","email":"7893459+eokoneyo@users.noreply.github.com"},"sourceCommit":{"committedDate":"2025-06-17T10:12:11Z","message":"Add export derivative integration type (#223994)\n\nThis PR adds a new share integration type (i.e. the `exportDerivative`).\nThis integration is a special one that is only to be used in a scenario\nwhere a team would like to offer some extra functionality that depends\non the availability of some `export` integration. Specifically because\nof the way this has been coupled to be presented to the user the\n`exportDerivative` type integration would only be displayed to the user\nwhen at least one export integration exists.\n\nThis particular integration is also much more flexible, in that it only\naccepts a label to denote what should be rendered to the user, and a\nfreeform component that will be rendered into the flyout that will in\nturn open when the provided label is clicked.\n\nOne might configure an integration for this like so;\n\n```ts\n share.registerShareIntegration<ExportShareDerivatives>({\n id: 'scheduledReporting',\n groupId: 'exportDerivatives',\n config: () => ({\n label: ({ openFlyout }) =>\n React.createElement(\n EuiButton,\n { iconType: 'calendar', onClick: openFlyout },\n 'schedule report'\n ),\n flyoutContent: ({ closeFlyout , flyoutRef }) => {\n return React.createElement(\n EuiButton,\n {\n iconType: 'calendar',\n },\n 'Click me'\n );\n },\n }),\n });\n```\n \n The following config would produce the following experience;\n \n \n\n\n\n","sha":"b275c9e9dc1ac74294a52d31b6037c3cf0d8d18c","branchLabelMapping":{"^v9.1.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","backport:skip","Team:SharedUX","v9.1.0"],"title":"Add export derivative integration type ","number":223994,"url":"https://github.com/elastic/kibana/pull/223994","mergeCommit":{"message":"Add export derivative integration type (#223994)\n\nThis PR adds a new share integration type (i.e. the `exportDerivative`).\nThis integration is a special one that is only to be used in a scenario\nwhere a team would like to offer some extra functionality that depends\non the availability of some `export` integration. Specifically because\nof the way this has been coupled to be presented to the user the\n`exportDerivative` type integration would only be displayed to the user\nwhen at least one export integration exists.\n\nThis particular integration is also much more flexible, in that it only\naccepts a label to denote what should be rendered to the user, and a\nfreeform component that will be rendered into the flyout that will in\nturn open when the provided label is clicked.\n\nOne might configure an integration for this like so;\n\n```ts\n share.registerShareIntegration<ExportShareDerivatives>({\n id: 'scheduledReporting',\n groupId: 'exportDerivatives',\n config: () => ({\n label: ({ openFlyout }) =>\n React.createElement(\n EuiButton,\n { iconType: 'calendar', onClick: openFlyout },\n 'schedule report'\n ),\n flyoutContent: ({ closeFlyout , flyoutRef }) => {\n return React.createElement(\n EuiButton,\n {\n iconType: 'calendar',\n },\n 'Click me'\n );\n },\n }),\n });\n```\n \n The following config would produce the following experience;\n \n \n\n\n\n","sha":"b275c9e9dc1ac74294a52d31b6037c3cf0d8d18c"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.1.0","branchLabelMappingKey":"^v9.1.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/223994","number":223994,"mergeCommit":{"message":"Add export derivative integration type (#223994)\n\nThis PR adds a new share integration type (i.e. the `exportDerivative`).\nThis integration is a special one that is only to be used in a scenario\nwhere a team would like to offer some extra functionality that depends\non the availability of some `export` integration. Specifically because\nof the way this has been coupled to be presented to the user the\n`exportDerivative` type integration would only be displayed to the user\nwhen at least one export integration exists.\n\nThis particular integration is also much more flexible, in that it only\naccepts a label to denote what should be rendered to the user, and a\nfreeform component that will be rendered into the flyout that will in\nturn open when the provided label is clicked.\n\nOne might configure an integration for this like so;\n\n```ts\n share.registerShareIntegration<ExportShareDerivatives>({\n id: 'scheduledReporting',\n groupId: 'exportDerivatives',\n config: () => ({\n label: ({ openFlyout }) =>\n React.createElement(\n EuiButton,\n { iconType: 'calendar', onClick: openFlyout },\n 'schedule report'\n ),\n flyoutContent: ({ closeFlyout , flyoutRef }) => {\n return React.createElement(\n EuiButton,\n {\n iconType: 'calendar',\n },\n 'Click me'\n );\n },\n }),\n });\n```\n \n The following config would produce the following experience;\n \n \n\n\n\n","sha":"b275c9e9dc1ac74294a52d31b6037c3cf0d8d18c"}}]}] BACKPORT-->
This PR adds a new share integration type (i.e. the
exportDerivative). This integration is a special one that is only to be used in a scenario where a team would like to offer some extra functionality that depends on the availability of someexportintegration. Specifically because of the way this has been coupled to be presented to the user theexportDerivativetype integration would only be displayed to the user when at least one export integration exists.This particular integration is also much more flexible, in that it only accepts a label to denote what should be rendered to the user, and a freeform component that will be rendered into the flyout that will in turn open when the provided label is clicked.
One might configure an integration for this like so;
The following config would produce the following experience;