From c1d70d364f6f2d9083265e9f7824aaa2185e90ba Mon Sep 17 00:00:00 2001 From: github-automation-metabase <166700802+github-automation-metabase@users.noreply.github.com> Date: Thu, 19 Dec 2024 16:02:00 -0500 Subject: [PATCH] Fix pdf exports issues (#51491) (#51540) * fix horizontally trimmed content on short pages * include filters * update specs Co-authored-by: Aleksandr Lesnenko <alxnddr@users.noreply.github.com> --- .../components/Dashboard/Dashboard.styled.tsx | 22 +++------ .../DashboardParameterPanel.tsx | 2 + frontend/src/metabase/dashboard/constants.ts | 2 + .../containers/AutomaticDashboardApp.jsx | 2 + .../components/EmbedFrame/EmbedFrame.tsx | 6 ++- .../visualizations/lib/save-dashboard-pdf.ts | 49 +++++++++++++------ .../lib/save-dashboard-pdf.unit.spec.ts | 27 ++++++++-- 7 files changed, 74 insertions(+), 36 deletions(-) diff --git a/frontend/src/metabase/dashboard/components/Dashboard/Dashboard.styled.tsx b/frontend/src/metabase/dashboard/components/Dashboard/Dashboard.styled.tsx index ae604503837..0f5d8c9ac0c 100644 --- a/frontend/src/metabase/dashboard/components/Dashboard/Dashboard.styled.tsx +++ b/frontend/src/metabase/dashboard/components/Dashboard/Dashboard.styled.tsx @@ -165,22 +165,6 @@ export const ParametersAndCardsContainer = styled.div<{ /* Makes sure it doesn't use all the height, so the actual content height could be used in embedding #37437 */ align-self: ${({ shouldMakeDashboardHeaderStickyAfterScrolling }) => !shouldMakeDashboardHeaderStickyAfterScrolling && "flex-start"}; - - &.${SAVING_DOM_IMAGE_CLASS} { - ${ParametersWidgetContainer} { - background-color: transparent; - border-bottom: none; - margin-top: 1rem; - - legend { - top: -12px; - } - } - - ${CardsContainer} { - padding-bottom: 20px; - } - } `; export const FIXED_WIDTH = "1048px"; @@ -195,6 +179,12 @@ export const FixedWidthContainer = styled.div<{ margin: 0 auto; max-width: ${FIXED_WIDTH}; `} + + .${SAVING_DOM_IMAGE_CLASS} & { + legend { + top: -9px; + } + } `; export const ParametersFixedWidthContainer = styled(FixedWidthContainer)` diff --git a/frontend/src/metabase/dashboard/components/DashboardParameterPanel/DashboardParameterPanel.tsx b/frontend/src/metabase/dashboard/components/DashboardParameterPanel/DashboardParameterPanel.tsx index d2cd9dc7b67..d7e17ffd811 100644 --- a/frontend/src/metabase/dashboard/components/DashboardParameterPanel/DashboardParameterPanel.tsx +++ b/frontend/src/metabase/dashboard/components/DashboardParameterPanel/DashboardParameterPanel.tsx @@ -1,3 +1,4 @@ +import { DASHBOARD_PARAMETERS_PDF_EXPORT_NODE_ID } from "metabase/dashboard/constants"; import { getDashboardComplete, getIsEditing, @@ -70,6 +71,7 @@ export function DashboardParameterPanel({ data-testid="dashboard-parameters-widget-container" > <ParametersFixedWidthContainer + id={DASHBOARD_PARAMETERS_PDF_EXPORT_NODE_ID} isFixedWidth={dashboard?.width === "fixed"} data-testid="fixed-width-filters" > diff --git a/frontend/src/metabase/dashboard/constants.ts b/frontend/src/metabase/dashboard/constants.ts index 3ab93944c8a..68c496e0386 100644 --- a/frontend/src/metabase/dashboard/constants.ts +++ b/frontend/src/metabase/dashboard/constants.ts @@ -53,6 +53,8 @@ export const DASHBOARD_SLOW_TIMEOUT = 15 * 1000; export const DASHBOARD_PDF_EXPORT_ROOT_ID = "Dashboard-Parameters-And-Cards-Container"; +export const DASHBOARD_PARAMETERS_PDF_EXPORT_NODE_ID = + "Dashboard-Parameters-Content"; export const DEFAULT_DASHBOARD_DISPLAY_OPTIONS: EmbedDisplayParams = { background: true, diff --git a/frontend/src/metabase/dashboard/containers/AutomaticDashboardApp.jsx b/frontend/src/metabase/dashboard/containers/AutomaticDashboardApp.jsx index 34bc05c4338..802b2f9c663 100644 --- a/frontend/src/metabase/dashboard/containers/AutomaticDashboardApp.jsx +++ b/frontend/src/metabase/dashboard/containers/AutomaticDashboardApp.jsx @@ -12,6 +12,7 @@ import Link from "metabase/core/components/Link"; import Tooltip from "metabase/core/components/Tooltip"; import CS from "metabase/css/core/index.css"; import { DashboardTabs } from "metabase/dashboard/components/DashboardTabs"; +import { DASHBOARD_PARAMETERS_PDF_EXPORT_NODE_ID } from "metabase/dashboard/constants"; import { Dashboard } from "metabase/dashboard/containers/Dashboard"; import { DashboardData } from "metabase/dashboard/hoc/DashboardData"; import { getIsHeaderVisible, getTabs } from "metabase/dashboard/selectors"; @@ -169,6 +170,7 @@ class AutomaticDashboardAppInner extends Component { {parameters && parameters.length > 0 && ( <div className={cx(CS.px1, CS.pt1)}> <FixedWidthContainer + id={DASHBOARD_PARAMETERS_PDF_EXPORT_NODE_ID} data-testid="fixed-width-filters" isFixedWidth={dashboard?.width === "fixed"} > diff --git a/frontend/src/metabase/public/components/EmbedFrame/EmbedFrame.tsx b/frontend/src/metabase/public/components/EmbedFrame/EmbedFrame.tsx index 84362831318..e9ed05e9b08 100644 --- a/frontend/src/metabase/public/components/EmbedFrame/EmbedFrame.tsx +++ b/frontend/src/metabase/public/components/EmbedFrame/EmbedFrame.tsx @@ -11,7 +11,10 @@ import { ParametersFixedWidthContainer, } from "metabase/dashboard/components/Dashboard/Dashboard.styled"; import { ExportAsPdfButton } from "metabase/dashboard/components/DashboardHeader/buttons/ExportAsPdfButton"; -import { DASHBOARD_PDF_EXPORT_ROOT_ID } from "metabase/dashboard/constants"; +import { + DASHBOARD_PARAMETERS_PDF_EXPORT_NODE_ID, + DASHBOARD_PDF_EXPORT_ROOT_ID, +} from "metabase/dashboard/constants"; import { getDashboardType } from "metabase/dashboard/utils"; import { initializeIframeResizer, isSmallScreen } from "metabase/lib/dom"; import { useSelector } from "metabase/lib/redux"; @@ -211,6 +214,7 @@ export const EmbedFrame = ({ data-testid="dashboard-parameters-widget-container" > <ParametersFixedWidthContainer + id={DASHBOARD_PARAMETERS_PDF_EXPORT_NODE_ID} data-testid="fixed-width-filters" isFixedWidth={dashboard?.width === "fixed"} > diff --git a/frontend/src/metabase/visualizations/lib/save-dashboard-pdf.ts b/frontend/src/metabase/visualizations/lib/save-dashboard-pdf.ts index f71afebb663..6f96468de66 100644 --- a/frontend/src/metabase/visualizations/lib/save-dashboard-pdf.ts +++ b/frontend/src/metabase/visualizations/lib/save-dashboard-pdf.ts @@ -1,6 +1,7 @@ import { t } from "ttag"; import _ from "underscore"; +import { DASHBOARD_PARAMETERS_PDF_EXPORT_NODE_ID } from "metabase/dashboard/constants"; import type { Dashboard } from "metabase-types/api"; import { SAVING_DOM_IMAGE_CLASS } from "./save-chart-image"; @@ -75,6 +76,7 @@ export const getPageBreaks = ( sortedCards: DashCardBounds[], optimalPageHeight: number, totalHeight: number, + minPageHeight: number, offset = 0, ): number[] => { if (sortedCards.length === 0) { @@ -88,7 +90,6 @@ export const getPageBreaks = ( return []; } - const minPageSize = optimalPageHeight * 0.7; const result: number[] = []; let currentPageStart = 0; let candidateIndex = 0; @@ -98,7 +99,7 @@ export const getPageBreaks = ( while ( candidateIndex < pageBreakCandidates.length && - pageBreakCandidates[candidateIndex] <= currentPageStart + minPageSize + pageBreakCandidates[candidateIndex] <= currentPageStart + minPageHeight ) { candidateIndex++; } @@ -141,7 +142,8 @@ const createHeaderElement = (dashboardName: string, marginBottom: number) => { return header; }; -const HEADER_MARGIN_BOTTOM = 8; +const HEADER_MARGIN_BOTTOM = 12; +const PARAMETERS_MARGIN_BOTTOM = 12; const PAGE_PADDING = 16; export const saveDashboardPdf = async ( @@ -149,25 +151,37 @@ export const saveDashboardPdf = async ( dashboardName: string, ) => { const fileName = `${dashboardName}.pdf`; - const node = document - .querySelector(selector) - ?.querySelector(".react-grid-layout"); + const dashboardRoot = document.querySelector(selector); + const gridNode = dashboardRoot?.querySelector(".react-grid-layout"); - if (!node || !(node instanceof HTMLElement)) { + if (!gridNode || !(gridNode instanceof HTMLElement)) { console.warn("No dashboard content found", selector); return; } - const cardsBounds = getSortedDashCardBounds(node); + const cardsBounds = getSortedDashCardBounds(gridNode); const pdfHeader = createHeaderElement(dashboardName, HEADER_MARGIN_BOTTOM); + const parametersNode = dashboardRoot + ?.querySelector(`#${DASHBOARD_PARAMETERS_PDF_EXPORT_NODE_ID}`) + ?.cloneNode(true); + + let parametersHeight = 0; + if (parametersNode instanceof HTMLElement) { + gridNode.append(parametersNode); + parametersNode.style.cssText = `margin-bottom: ${PARAMETERS_MARGIN_BOTTOM}px`; + parametersHeight = + parametersNode.getBoundingClientRect().height + PARAMETERS_MARGIN_BOTTOM; + gridNode.removeChild(parametersNode); + } - node.appendChild(pdfHeader); + gridNode.appendChild(pdfHeader); const headerHeight = pdfHeader.getBoundingClientRect().height + HEADER_MARGIN_BOTTOM; - node.removeChild(pdfHeader); + gridNode.removeChild(pdfHeader); - const contentWidth = node.offsetWidth; - const contentHeight = node.offsetHeight + headerHeight; + const verticalOffset = headerHeight + parametersHeight; + const contentWidth = gridNode.offsetWidth; + const contentHeight = gridNode.offsetHeight + verticalOffset; const width = contentWidth + PAGE_PADDING * 2; const backgroundColor = getComputedStyle(document.documentElement) @@ -175,7 +189,7 @@ export const saveDashboardPdf = async ( .trim(); const { default: html2canvas } = await import("html2canvas-pro"); - const image = await html2canvas(node, { + const image = await html2canvas(gridNode, { height: contentHeight, width: contentWidth, useCORS: true, @@ -183,18 +197,25 @@ export const saveDashboardPdf = async ( node.classList.add(SAVING_DOM_IMAGE_CLASS); node.style.height = `${contentHeight}px`; node.style.backgroundColor = backgroundColor; + if (parametersNode instanceof HTMLElement) { + node.insertBefore(parametersNode, node.firstChild); + } node.insertBefore(pdfHeader, node.firstChild); }, }); const { default: jspdf } = await import("jspdf"); + // Page page height cannot be smaller than page width otherwise the content will be cut off + // or the page should have a landscape orientation. + const minPageHeight = contentWidth; const optimalPageHeight = Math.round(width * TARGET_ASPECT_RATIO); const pageBreaks = getPageBreaks( cardsBounds, optimalPageHeight - PAGE_PADDING * 2, contentHeight, - headerHeight, + minPageHeight, + verticalOffset, ); const pdf = new jspdf({ diff --git a/frontend/src/metabase/visualizations/lib/save-dashboard-pdf.unit.spec.ts b/frontend/src/metabase/visualizations/lib/save-dashboard-pdf.unit.spec.ts index 95dc7db0f40..53ace3a158a 100644 --- a/frontend/src/metabase/visualizations/lib/save-dashboard-pdf.unit.spec.ts +++ b/frontend/src/metabase/visualizations/lib/save-dashboard-pdf.unit.spec.ts @@ -94,7 +94,7 @@ describe("save-dashboard-pdf", () => { }, ]; - const breaks = getPageBreaks(cards, 300, 200); + const breaks = getPageBreaks(cards, 300, 200, 100); expect(breaks).toEqual([]); }); @@ -111,7 +111,7 @@ describe("save-dashboard-pdf", () => { ]; const optimalPageHeight = 270; - const breaks = getPageBreaks(cards, optimalPageHeight, 500); + const breaks = getPageBreaks(cards, optimalPageHeight, 500, 100); expect(breaks).toEqual([250]); // The page break at 250 is the closest to the optimal page height of 270 }); @@ -132,11 +132,11 @@ describe("save-dashboard-pdf", () => { }, ]; - const breaks = getPageBreaks(cards, 200, 600); + const breaks = getPageBreaks(cards, 200, 600, 100); expect(breaks).toEqual([200, 400]); }); - it("should respect minimum page size constraint", () => { + it("should allow breaks that respect minimum page size", () => { const cards = [ { top: 0, bottom: 100, height: 100, allowedBreaks: new Set<number>() }, { @@ -148,8 +148,25 @@ describe("save-dashboard-pdf", () => { { top: 250, bottom: 300, height: 50, allowedBreaks: new Set<number>() }, ]; - const breaks = getPageBreaks(cards, 200, 300); + const breaks = getPageBreaks(cards, 200, 300, 100); expect(breaks).toEqual([250]); }); + + it("should not break if it would create pages smaller than minimum size", () => { + const cards = [ + { top: 0, bottom: 150, height: 150, allowedBreaks: new Set<number>() }, + { + top: 150, + bottom: 400, + height: 250, + allowedBreaks: new Set<number>(), + }, + ]; + + // With minPageHeight of 200, breaking at 150 would create a first page + // of only 150px height, which is less than minimum, so no breaks should occur + const breaks = getPageBreaks(cards, 250, 400, 200); + expect(breaks).toEqual([]); + }); }); }); -- GitLab