rusackas commented on code in PR #38535:
URL: https://github.com/apache/superset/pull/38535#discussion_r3523799895
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:
##########
@@ -247,29 +253,80 @@ const SliceHeaderControls = (
props.exportXLSX?.(props.slice.slice_id);
break;
case MenuKeys.DownloadAsImage: {
- // menu closes with a delay, we need to hide it manually,
- // so that we don't capture it on the screenshot
- const menu = document.querySelector(
- '.ant-dropdown:not(.ant-dropdown-hidden)',
- ) as HTMLElement;
- if (menu) {
- menu.style.visibility = 'hidden';
+ // Hide the dropdown overlay so it is not captured in the screenshot
+ const overlayContainer = dropdownOverlayContainerRef.current;
+ if (overlayContainer) {
+ overlayContainer.style.visibility = 'hidden';
}
downloadAsImage(
getScreenshotNodeSelector(props.slice.slice_id),
props.slice.slice_name,
true,
theme,
)(domEvent).then(() => {
- if (menu) {
- menu.style.visibility = 'visible';
+ if (overlayContainer) {
+ overlayContainer.style.visibility = 'visible';
}
});
+ // eslint-disable-next-line no-unused-expressions
props.logEvent?.(LOG_ACTIONS_CHART_DOWNLOAD_AS_IMAGE, {
chartId: props.slice.slice_id,
});
break;
}
+ case MenuKeys.DownloadAsPngTransparent:
+ case MenuKeys.DownloadAsPngSolid: {
+ const overlayContainer = dropdownOverlayContainerRef.current;
+ if (overlayContainer) {
+ overlayContainer.style.visibility = 'hidden';
+ }
+
+ const backgroundType =
+ key === MenuKeys.DownloadAsPngTransparent ? 'transparent' : 'solid';
+
+ downloadAsImage(
+ getScreenshotNodeSelector(props.slice.slice_id),
+ props.slice.slice_name,
+ true,
+ theme,
+ { format: 'png', backgroundType },
+ )(domEvent).then(() => {
+ if (overlayContainer) {
+ overlayContainer.style.visibility = 'visible';
+ }
+ });
+
+ // eslint-disable-next-line no-unused-expressions
+ props.logEvent?.(LOG_ACTIONS_CHART_DOWNLOAD_AS_PNG, {
+ chartId: props.slice.slice_id,
+ backgroundType,
+ });
+ break;
+ }
+ case MenuKeys.DownloadAsPdf: {
+ const overlayContainer = dropdownOverlayContainerRef.current;
+ if (overlayContainer) {
+ overlayContainer.style.visibility = 'hidden';
+ }
+
+ const pdfResult = downloadAsPdf(
+ getScreenshotNodeSelector(props.slice.slice_id),
+ props.slice.slice_name,
+ true,
+ )(domEvent);
+
+ Promise.resolve(pdfResult).then(() => {
+ if (overlayContainer) {
+ overlayContainer.style.visibility = 'visible';
+ }
+ });
+
+ // eslint-disable-next-line no-unused-expressions
+ props.logEvent?.(LOG_ACTIONS_CHART_DOWNLOAD_AS_PDF, {
+ chartId: props.slice.slice_id,
+ });
+ break;
+ }
Review Comment:
This one's real... the restore belongs in a `.finally()` so a failed capture
doesn't leave the menu permanently hidden. Worth applying to all three new
handlers during the rebase.
##########
superset-frontend/src/dashboard/components/SliceHeaderControls/index.tsx:
##########
@@ -547,15 +604,44 @@ const SliceHeaderControls = (
: []),
{
key: MenuKeys.DownloadAsImage,
- label: t('Download as image'),
+ label: t('Export screenshot (jpeg)'),
+ icon: <Icons.FileImageOutlined css={dropdownIconsStyles} />,
+ },
+ {
+ type: 'submenu',
+ key: 'download_as_png_submenu',
+ label: t('Export screenshot (PNG)'),
icon: <Icons.FileImageOutlined css={dropdownIconsStyles} />,
+ children: [
+ {
+ key: MenuKeys.DownloadAsPngTransparent,
+ label: t('Transparent background'),
+ },
+ {
+ key: MenuKeys.DownloadAsPngSolid,
+ label: t('Solid background'),
+ },
+ ],
+ },
+ {
+ key: MenuKeys.DownloadAsPdf,
+ label: t('Export as PDF'),
+ icon: <Icons.FileOutlined css={dropdownIconsStyles} />,
},
],
});
}
return (
<>
+ <div
+ ref={dropdownOverlayContainerRef}
+ data-slice-header-dropdown-container
+ css={css`
+ position: absolute;
+ z-index: 0;
Review Comment:
Fair concern. Since the existing `.ant-dropdown` visibility hack on master
still works, the simplest fix might be dropping the custom popup container
entirely and reusing that pattern for the new cases.
##########
superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/index.tsx:
##########
@@ -833,6 +934,79 @@ export const useExploreAdditionalActionsMenu = (
);
},
},
+ {
+ type: 'submenu' as const,
+ key: 'export_current_png_submenu',
+ label: t('Export screenshot (png)'),
+ icon: <Icons.FileImageOutlined />,
+ children: [
+ {
+ key: MENU_KEYS.EXPORT_CURRENT_PNG_TRANSPARENT,
+ label: t('Transparent background'),
+ onClick: (e: {
+ domEvent: React.MouseEvent | React.KeyboardEvent;
+ }) => {
+ downloadAsImage(
+ '.panel-body .chart-container',
+ slice?.slice_name ?? t('New chart'),
+ true,
+ theme,
+ { format: 'png', backgroundType: 'transparent' },
+ )(e.domEvent);
Review Comment:
Agreed, though the cleaner fix is reusing `getExportScreenshotMenuItems`
here per Joe's comment... that resolves the selector duplication as a side
effect.
##########
superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/index.tsx:
##########
@@ -833,6 +934,79 @@ export const useExploreAdditionalActionsMenu = (
);
},
},
+ {
+ type: 'submenu' as const,
+ key: 'export_current_png_submenu',
+ label: t('Export screenshot (png)'),
+ icon: <Icons.FileImageOutlined />,
+ children: [
+ {
+ key: MENU_KEYS.EXPORT_CURRENT_PNG_TRANSPARENT,
+ label: t('Transparent background'),
+ onClick: (e: {
+ domEvent: React.MouseEvent | React.KeyboardEvent;
+ }) => {
+ downloadAsImage(
+ '.panel-body .chart-container',
+ slice?.slice_name ?? t('New chart'),
+ true,
+ theme,
+ { format: 'png', backgroundType: 'transparent' },
+ )(e.domEvent);
+ setIsDropdownVisible(false);
+ dispatch(
+ logEvent(LOG_ACTIONS_CHART_DOWNLOAD_AS_PNG, {
+ chartId: slice?.slice_id,
+ chartName: slice?.slice_name,
+ backgroundType: 'transparent',
+ }),
+ );
+ },
+ },
+ {
+ key: MENU_KEYS.EXPORT_CURRENT_PNG_SOLID,
+ label: t('Solid background'),
+ onClick: (e: {
+ domEvent: React.MouseEvent | React.KeyboardEvent;
+ }) => {
+ downloadAsImage(
+ '.panel-body .chart-container',
+ slice?.slice_name ?? t('New chart'),
+ true,
+ theme,
+ { format: 'png', backgroundType: 'solid' },
+ )(e.domEvent);
Review Comment:
Same as the thread above... goes away once the menu items are DRYed up.
##########
superset-frontend/src/explore/components/useExploreAdditionalActionsMenu/index.tsx:
##########
@@ -833,6 +934,79 @@ export const useExploreAdditionalActionsMenu = (
);
},
},
+ {
+ type: 'submenu' as const,
+ key: 'export_current_png_submenu',
+ label: t('Export screenshot (png)'),
+ icon: <Icons.FileImageOutlined />,
+ children: [
+ {
+ key: MENU_KEYS.EXPORT_CURRENT_PNG_TRANSPARENT,
+ label: t('Transparent background'),
+ onClick: (e: {
+ domEvent: React.MouseEvent | React.KeyboardEvent;
+ }) => {
+ downloadAsImage(
+ '.panel-body .chart-container',
+ slice?.slice_name ?? t('New chart'),
+ true,
+ theme,
+ { format: 'png', backgroundType: 'transparent' },
+ )(e.domEvent);
+ setIsDropdownVisible(false);
+ dispatch(
+ logEvent(LOG_ACTIONS_CHART_DOWNLOAD_AS_PNG, {
+ chartId: slice?.slice_id,
+ chartName: slice?.slice_name,
+ backgroundType: 'transparent',
+ }),
+ );
+ },
+ },
+ {
+ key: MENU_KEYS.EXPORT_CURRENT_PNG_SOLID,
+ label: t('Solid background'),
+ onClick: (e: {
+ domEvent: React.MouseEvent | React.KeyboardEvent;
+ }) => {
+ downloadAsImage(
+ '.panel-body .chart-container',
+ slice?.slice_name ?? t('New chart'),
+ true,
+ theme,
+ { format: 'png', backgroundType: 'solid' },
+ )(e.domEvent);
+ setIsDropdownVisible(false);
+ dispatch(
+ logEvent(LOG_ACTIONS_CHART_DOWNLOAD_AS_PNG, {
+ chartId: slice?.slice_id,
+ chartName: slice?.slice_name,
+ backgroundType: 'solid',
+ }),
+ );
+ },
+ },
+ ],
+ },
+ {
+ key: MENU_KEYS.EXPORT_CURRENT_PDF,
+ label: t('Export as PDF'),
+ icon: <Icons.FileOutlined />,
+ onClick: (e: { domEvent: React.MouseEvent | React.KeyboardEvent }) => {
+ downloadAsPdf(
+ '.panel-body .chart-container',
+ slice?.slice_name ?? t('New chart'),
+ true,
+ )(e.domEvent);
Review Comment:
Same as above, covered by the DRY fix.
##########
superset-frontend/src/features/alerts/AlertReportModal.test.tsx:
##########
@@ -731,9 +731,9 @@ test('opens Schedule Section on click', async () => {
useRedux: true,
});
userEvent.click(screen.getByTestId('schedule-panel'));
- const scheduleHeader = within(
+ const [scheduleHeader] = within(
screen.getByRole('tab', { expanded: true }),
Review Comment:
This assertion predates the PR... the diff just destructures the existing
`queryAllByText` call without changing what it matches. Tightening it would be
a separate cleanup.
##########
superset-frontend/src/features/alerts/AlertReportModal.test.tsx:
##########
@@ -799,9 +799,9 @@ test('opens Notification Method Section on click', async ()
=> {
useRedux: true,
});
userEvent.click(screen.getByTestId('notification-method-panel'));
- const notificationMethodHeader = within(
+ const [notificationMethodHeader] = within(
screen.getByRole('tab', { expanded: true }),
Review Comment:
Same as the schedule-field thread... pre-existing assertion, out of scope
here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
