sadpandajoe commented on code in PR #44154:
URL: https://github.com/apache/superset/pull/44154#discussion_r4169145586
##########
superset-frontend/src/utils/downloadAsPdf.ts:
##########
@@ -46,27 +46,38 @@ const generateFileStem = (description: string, date = new
Date()) =>
* @param description name or a short description of what is being printed.
* Value will be normalized, and a date as well as a file extension will be
added.
* @param isExactSelector if false, searches for the closest ancestor that
matches selector.
+ * @param addWarningToast bound via `useToasts()`/`bindActionCreators`, not
the raw
+ * action creator from `actions.ts`: this module has no dispatch of its own,
so an
+ * unbound creator would only build a Redux action object and never render a
toast.
+ * @param addInfoToast same contract as `addWarningToast`; announces a
multi-batch
+ * export while charts are being forced into view.
* @returns event handler
*/
export default function downloadAsPdf(
selector: string,
description: string,
isExactSelector = false,
+ addWarningToast?: (message: string) => void,
+ addInfoToast?: (message: string) => void,
) {
return async (event: SyntheticEvent) => {
const elementToPrint = isExactSelector
? document.querySelector(selector)
: event.currentTarget.closest(selector);
if (!elementToPrint) {
- return dispatchWarningToast(
- t('PDF download failed, please refresh and try again.'),
- );
+ addWarningToast?.(PDF_DOWNLOAD_FAILED_MESSAGE);
+ return;
}
// Force any virtualized (unmounted) charts to render before capturing, so
// off-screen rows are not exported as loading spinners.
- const didForceLoad = await forceLoadAllCharts(elementToPrint);
+ const didForceLoad = await forceLoadAllCharts(
+ elementToPrint,
+ undefined,
Review Comment:
Removing these forwarded callbacks would silently drop the
preparation/timeout toasts without failing the existing tests: the menu tests
mock the exporters, the PDF tests mock `forceLoadAllCharts` without checking
its arguments, and the image tests never enable virtualization. Could
exporter-level tests assert that both bound callbacks reach
`forceLoadAllCharts` for image and PDF exports?
--
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]
