-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Share Modal Redesign] Reporting modals with licensing using link/embed tabbed modal component #179206
Conversation
/ci |
💔 Build FailedFailed CI Steps
Test Failures
Metrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: cc @rshen91 |
x-pack/plugins/lens/public/plugin.ts
Outdated
downloadCsvShareProvider({ | ||
uiSettings: core.uiSettings, | ||
formatFactoryFn: () => startServices().plugins.fieldFormats.deserialize, | ||
license, |
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.
It looks like the CSV Download option is getting removed unless the license type is at least Gold. It would be better to handle that here, and use the license type as a condition to suppress registration of the option completely.
const startServices$ = from(getStartServices());
startServices$.subscribe(([, { licensing }]) => {
licensing?.license$.subscribe((license) => {
- return share.register(
- downloadCsvShareProvider({
- uiSettings: core.uiSettings,
- formatFactoryFn: () => startServices().plugins.fieldFormats.deserialize,
- license,
- })
- );
+ const atLeastGold = license.hasAtLeast('gold');
+ if (atLeastGold) {
+ share.register(
+ downloadCsvShareProvider({
+ uiSettings: core.uiSettings,
+ formatFactoryFn: () => startServices().plugins.fieldFormats.deserialize,
+ })
+ );
+ }
});
});
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.
oh I love this thank you
? [ | ||
{ id: 'printablePdfV2', label: 'PDF' }, | ||
{ id: 'pngV2', label: 'PNG', 'data-test-subj': 'pngReportOption' }, | ||
{ id: 'csv', label: 'CSV', 'data-test-subj': 'lensCSVReport' }, |
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.
Lens CSV Report doesn't belong here
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.
Ok so I think I'm following based on the comment #179206 (comment) and keeping in mind the designs in the figma, I would want to set up the modal to have the registerModalOption(exportCsv, exportPdf, exportPng)
? Because Lens will have to have all three types in the same export...For the other cases I think I'm following. Thanks!
// needed for canvas | ||
getJobParams?: JobAppParamsPDFV2; | ||
objectType: string; | ||
downloadCsvFromLens: () => 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.
"Download CSV From Lens" doesn't belong here
|
||
export type Props = ReportingModalProps & { intl: InjectedIntl }; | ||
|
||
type AllowedImageExportType = 'pngV2' | 'printablePdfV2' | 'printablePdf' | 'csv'; |
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.
csv
isn't an image type
*printablePdf
is deprecated, so is only available via API, not the UI
type AllowedImageExportType = 'pngV2' | 'printablePdfV2' | 'printablePdf' | 'csv'; | |
type AllowedImageExportType = 'pngV2' | 'printablePdfV2'; |
The PR needs to support everything the way it was by default, when With const { share: shareSetup } = somethingPluginSetupDeps;
if (isNewVersionEnabled) {
shareSetup.registerModalOption(somethingShareModalOptionProvider()); // new functionality
} else {
shareSetup.register(somethingShareProvider()); // existing functionality
} In the modal, each sharing tab is responsible for showing certain kinds of options. This doesn't overlap with the old way, where the sharing menu shows all the components that get registered. In the new way, the objects that get registered should describe a component and that component will have to be brought to life in the sharing tab. I recommend defining a new type of object ( interface ShareModalOption {
/*
* Define which tab the option belongs to
*/
tabType: 'links' | 'export';
/*
* When the option is selected, the header shows intro about the option.
* If there is a single option, the header is static.
*/
headerWhenSelected: string;
/*
* When the option is selected, the footer shows a primary button and optionally a secondary button
* If there is a single option, the footer is static.
*/
footerWhenSelected: Array<{ type: 'primaryBtn' | 'secondaryBtn'; text: string }>;
/*
* Control the actions that happen when the option is selected, such as copy the link, download the CSV, or
* send a request to generate a report. Make sure the action calls a close() method if the modal should be
* closed after the action happens.
*/
action: Function;
/*
* The sharable link option is "single"
* The export options use "radio"
*/
optionType: 'single' | 'radio';
/*
* If there are radio options in the tab, use this to show the label above the options
*/
radioType?: 'fileType';
/*
* The Sharing Link option will be a JSX.Element showing the link text
* The Export options will be a string, such as "PDF", "PNG", "CSV"
*/
content: string | JSX.Element;
}
/*
* built-in to share plugin?
*/
const copyLink: ShareModalOption = {
tabType: 'links',
headerWhenSelected: 'Share a direct link to this lens.',
footerWhenSelected: [{ type: 'primaryBtn', text: 'Copy link' }],
action: () => {
/* call the copy-to-clipboard code */
},
optionType: 'single',
content: <>{/* component to show the text with copyable link */}</>,
};
shareSetup.registerModalOption(copyLink);
/*
* lens/public/plugin.ts
*/
const downloadCsv: ShareModalOption = {
tabType: 'export',
headerWhenSelected: 'Download the data displayed in the visualization.',
footerWhenSelected: [{ type: 'primaryBtn', text: 'Download CSV' }],
action: () => {
/* call the download CSV code */
},
optionType: 'radio',
radioType: 'fileType',
content: <>CSV</>,
};
shareSetup.registerModalOption(downloadCsv);
/*
* reporting/public/plugin.ts
*/
const exportPdf: ShareModalOption = {
tabType: 'export',
headerWhenSelected: 'Exports can take a few minutes to generate.',
footerWhenSelected: [
{ type: 'secondaryBtn', text: 'Copy POST URL' },
{ type: 'primaryBtn', text: 'Generate report' },
],
action: () => {
/* call the generate PDF code */
},
optionType: 'radio',
radioType: 'fileType',
content: <>PDF</>,
};
const exportPng: ShareModalOption = {
tabType: 'export',
headerWhenSelected: 'Exports can take a few minutes to generate.',
footerWhenSelected: [
{ type: 'secondaryBtn', text: 'Copy POST URL' },
{ type: 'primaryBtn', text: 'Generate report' },
],
action: () => {
/* call the generate PNG code */
},
optionType: 'radio',
radioType: 'fileType',
content: <>PNG</>,
};
shareSetup.registerModalOption(exportPdf);
shareSetup.registerModalOption(exportPng); When these types of objects are registered, the Sharing Modal has the job retrieve them and "bring them to life" in the tab, by picking the options that belong to the tab, showing the content, updating things based on the selected option, and calling the action when the buttons are clicked. This interface is just a starting point. You might need more fields, such as an action function for "secondary buttons," and for fine-tuning things like the ordering of radio options, putting in icons, etc. I hope this helps! |
Closing in favor of PR #180009 |
## Summary This PR refactors #179206 to have each export type be registered in Reporting and then passed into the share plugin. This PR is focused on the redesign in terms of the export modals. Test refactoring will be done in a separate PR. Partially closes elastic/kibana-team#753 - [x] Need to refactor this PR to include @eokoneyo's general modal component - [x] Lens needs to have Export with all three report type options - to avoid circular dependencies move the Lens CSV stuff into the reporting plugin vs having it in Lens - [x] Canvas should not be affected by these changes (so the old share/reporting code has to stay for canvas) #151523 to keep in mind for the redesign Failed tests will be covered in this PR #180406 ### TO TEST Mark `share.new_version.enabled: true` in your kibana.dev.yml ### Checklist - [x] 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/packages/kbn-i18n/README.md) - [x] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) --------- Co-authored-by: Eyo Okon Eyo <eyo.eyo@elastic.co> Co-authored-by: Tim Sullivan <tsullivan@users.noreply.github.com> Co-authored-by: Marco Liberati <dej611@users.noreply.github.com>
Summary
This PR is focused on the redesign in terms of the export modals. Test refactoring will be done in a separate PR.
Partially closes https://github.com/elastic/kibana-team/issues/753
[Discover] Unsaved work error on CSV reports for read-only user #151523 to keep in mind for the redesign
Failed tests will be covered in this PR #179694
To test:
Add
share.new_version.enabled: true
to your kibana.dev.ymlChecklist
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support