From ed80de02c85a10844f0b5f39201251b0363c2f8b Mon Sep 17 00:00:00 2001 From: Paul Marbach Date: Thu, 28 Aug 2025 10:47:26 -0400 Subject: [PATCH] change to Max row height instead of Max cell height --- .../panel-table/table_kitchen_sink.json | 4 --- .../panels-suite/table-kitchenSink.spec.ts | 35 ++++++------------- .../panels-suite/table-markdown.spec.ts | 23 +++++++++++- e2e-playwright/panels-suite/table-utils.ts | 13 +++++++ .../grafana-schema/src/common/common.gen.ts | 4 --- packages/grafana-schema/src/common/table.cue | 2 -- .../panelcfg/x/TablePanelCfg_types.gen.ts | 4 +++ .../Table/TableNG/Cells/AutoCell.tsx | 17 ++++----- .../src/components/Table/TableNG/TableNG.tsx | 29 +++++++++------ .../src/components/Table/TableNG/hooks.ts | 7 +++- .../src/components/Table/TableNG/types.ts | 1 + .../src/components/Table/TableNG/utils.ts | 16 ++++----- public/app/plugins/panel/table/TablePanel.tsx | 1 + public/app/plugins/panel/table/module.tsx | 19 +++++----- public/app/plugins/panel/table/panelcfg.cue | 10 +++--- .../app/plugins/panel/table/panelcfg.gen.ts | 4 +++ public/locales/en-US/grafana.json | 2 +- 17 files changed, 111 insertions(+), 80 deletions(-) create mode 100644 e2e-playwright/panels-suite/table-utils.ts diff --git a/devenv/dev-dashboards/panel-table/table_kitchen_sink.json b/devenv/dev-dashboards/panel-table/table_kitchen_sink.json index 44c37b2ae71..2b9ab7817cf 100644 --- a/devenv/dev-dashboards/panel-table/table_kitchen_sink.json +++ b/devenv/dev-dashboards/panel-table/table_kitchen_sink.json @@ -295,10 +295,6 @@ "wrapText": true } }, - { - "id": "custom.maxHeight", - "value": null - }, { "id": "custom.width", "value": 255 diff --git a/e2e-playwright/panels-suite/table-kitchenSink.spec.ts b/e2e-playwright/panels-suite/table-kitchenSink.spec.ts index 1a66796e84c..81e207bec9e 100644 --- a/e2e-playwright/panels-suite/table-kitchenSink.spec.ts +++ b/e2e-playwright/panels-suite/table-kitchenSink.spec.ts @@ -2,6 +2,8 @@ import { Page, Locator } from '@playwright/test'; import { test, expect, E2ESelectorGroups } from '@grafana/plugin-e2e'; +import { getCell, getCellHeight } from './table-utils'; + const DASHBOARD_UID = 'dcb9f5e9-8066-4397-889e-864b99555dbb'; test.use({ viewport: { width: 2000, height: 1080 } }); @@ -11,18 +13,6 @@ const waitForTableLoad = async (loc: Page | Locator) => { await expect(loc.locator('.rdg')).toBeVisible(); }; -const getCell = async (loc: Page | Locator, rowIdx: number, colIdx: number) => - loc - .getByRole('row') - .nth(rowIdx) - .getByRole(rowIdx === 0 ? 'columnheader' : 'gridcell') - .nth(colIdx); - -const getCellHeight = async (loc: Page | Locator, rowIdx: number, colIdx: number) => { - const cell = await getCell(loc, rowIdx, colIdx); - return (await cell.boundingBox())?.height ?? 0; -}; - const getColumnIdx = async (loc: Page | Locator, columnName: string) => { // find the index of the column "Long text." The kitchen sink table will change over time, but // we can just find the column programatically and use it throughout the test. @@ -77,22 +67,19 @@ test.describe('Panels test: Table - Kitchen Sink', { tag: ['@panels', '@table'] // text wrapping is enabled by default on this panel. await expect(getCellHeight(page, 1, longTextColIdx)).resolves.toBeGreaterThan(100); - // toggle the lorem ipsum column's wrap text toggle and confirm that the height shrinks. - const longTextFieldOverrides = dashboardPage.getByGrafanaSelector( - selectors.components.OptionsGroup.group('panel-options-override-12') - ); - - // TODO: we have added a null value for max height to the field overrides in the JSON, - // because there's no good way to add a field override in an e2e at this point. - const maxCellHeightInput = longTextFieldOverrides.getByLabel('Max cell height'); - - await maxCellHeightInput.fill('80'); + // set a max row height, watch the height decrease, then clear it to continue. + const maxRowHeightInput = page.getByLabel('Max row height').last(); + await maxRowHeightInput.fill('80'); await expect(async () => { await expect(getCellHeight(page, 1, longTextColIdx)).resolves.toBeLessThan(100); }).toPass(); - await maxCellHeightInput.clear(); + await maxRowHeightInput.clear(); - await longTextFieldOverrides.getByLabel('Wrap text').click({ force: true }); + // toggle the lorem ipsum column's wrap text toggle and confirm that the height shrinks. + await dashboardPage + .getByGrafanaSelector(selectors.components.OptionsGroup.group('panel-options-override-12')) + .getByLabel('Wrap text') + .click({ force: true }); await expect(getCellHeight(page, 1, longTextColIdx)).resolves.toBeLessThan(100); // test that hover overflow works. diff --git a/e2e-playwright/panels-suite/table-markdown.spec.ts b/e2e-playwright/panels-suite/table-markdown.spec.ts index 68068271fe6..02295359cf6 100644 --- a/e2e-playwright/panels-suite/table-markdown.spec.ts +++ b/e2e-playwright/panels-suite/table-markdown.spec.ts @@ -1,9 +1,13 @@ import { test, expect } from '@grafana/plugin-e2e'; +import { getCellHeight } from './table-utils'; + test.use({ viewport: { width: 1280, height: 1080 }, }); +const MARKDOWN_DASHBOARD_UID = '2769f5d8-0094-4ac4-a4f0-f68f620339cc'; + test.describe( 'Panels test: Table - Markdown', { @@ -12,11 +16,28 @@ test.describe( () => { test('Tests Markdown tables are successfully rendered', async ({ gotoDashboardPage, page }) => { await gotoDashboardPage({ - uid: '2769f5d8-0094-4ac4-a4f0-f68f620339cc', + uid: MARKDOWN_DASHBOARD_UID, queryParams: new URLSearchParams({ editPanel: '1' }), }); await expect(page.getByRole('grid')).toBeVisible(); }); + + test('Tests dynamic height and max row height', async ({ gotoDashboardPage, page }) => { + await gotoDashboardPage({ + uid: MARKDOWN_DASHBOARD_UID, + queryParams: new URLSearchParams({ editPanel: '1' }), + }); + + // confirm that the second row of the table is tall due to the content in it + await expect(getCellHeight(page, 2, 1)).resolves.toBeGreaterThan(100); + + // set the max row height to 80, watch the row shrink + const maxRowHeightInput = page.getByLabel('Max row height').last(); + await maxRowHeightInput.fill('80'); + await expect(async () => { + await expect(getCellHeight(page, 2, 1)).resolves.toBeLessThan(100); + }).toPass(); + }); } ); diff --git a/e2e-playwright/panels-suite/table-utils.ts b/e2e-playwright/panels-suite/table-utils.ts new file mode 100644 index 00000000000..56e04597bda --- /dev/null +++ b/e2e-playwright/panels-suite/table-utils.ts @@ -0,0 +1,13 @@ +import { Page, Locator } from '@playwright/test'; + +export const getCell = async (loc: Page | Locator, rowIdx: number, colIdx: number) => + loc + .getByRole('row') + .nth(rowIdx) + .getByRole(rowIdx === 0 ? 'columnheader' : 'gridcell') + .nth(colIdx); + +export const getCellHeight = async (loc: Page | Locator, rowIdx: number, colIdx: number) => { + const cell = await getCell(loc, rowIdx, colIdx); + return (await cell.boundingBox())?.height ?? 0; +}; diff --git a/packages/grafana-schema/src/common/common.gen.ts b/packages/grafana-schema/src/common/common.gen.ts index e847cd43c4f..8d7c5f85bef 100644 --- a/packages/grafana-schema/src/common/common.gen.ts +++ b/packages/grafana-schema/src/common/common.gen.ts @@ -1009,10 +1009,6 @@ export interface TableFieldOptions extends HideableFieldConfig { */ hideHeader?: boolean; inspect: boolean; - /** - * if set, limit the height in pixels that the wrapped text can flow to - */ - maxHeight?: number; minWidth?: number; /** * Selecting or hovering this field will show a tooltip containing the content within the target field diff --git a/packages/grafana-schema/src/common/table.cue b/packages/grafana-schema/src/common/table.cue index 8b96bad7aca..91def1fa980 100644 --- a/packages/grafana-schema/src/common/table.cue +++ b/packages/grafana-schema/src/common/table.cue @@ -131,8 +131,6 @@ TableFieldOptions: { hideHeader?: bool // if true, wrap the text content of the cell wrapText?: bool - // if set, limit the height in pixels that the wrapped text can flow to - maxHeight?: number // Enables text wrapping for column headers wrapHeaderText?: bool // Selecting or hovering this field will show a tooltip containing the content within the target field diff --git a/packages/grafana-schema/src/raw/composable/table/panelcfg/x/TablePanelCfg_types.gen.ts b/packages/grafana-schema/src/raw/composable/table/panelcfg/x/TablePanelCfg_types.gen.ts index 1aa6ed61a7e..f287e4ab245 100644 --- a/packages/grafana-schema/src/raw/composable/table/panelcfg/x/TablePanelCfg_types.gen.ts +++ b/packages/grafana-schema/src/raw/composable/table/panelcfg/x/TablePanelCfg_types.gen.ts @@ -31,6 +31,10 @@ export interface Options { frozenColumns?: { left?: number; }; + /** + * limits the maximum height of a row, if text wrapping or dynamic height is enabled + */ + maxRowHeight?: number; /** * Controls whether the panel should show the header */ diff --git a/packages/grafana-ui/src/components/Table/TableNG/Cells/AutoCell.tsx b/packages/grafana-ui/src/components/Table/TableNG/Cells/AutoCell.tsx index 24774d9df91..fb5d9fe4a15 100644 --- a/packages/grafana-ui/src/components/Table/TableNG/Cells/AutoCell.tsx +++ b/packages/grafana-ui/src/components/Table/TableNG/Cells/AutoCell.tsx @@ -24,14 +24,15 @@ export const getStyles: TableCellStyles = (_theme, { textWrap, shouldOverflow, m whiteSpace: 'pre-line', }, }), - ...(typeof maxHeight === 'number' && { - height: 'auto', - minHeight: 'none', - overflowY: 'hidden', - display: '-webkit-box', - WebkitBoxOrient: 'vertical', - WebkitLineClamp: Math.floor(maxHeight / TABLE.LINE_HEIGHT), - }), + ...(maxHeight != null && + textWrap && { + height: 'auto', + minHeight: 'none', + overflowY: 'hidden', + display: '-webkit-box', + WebkitBoxOrient: 'vertical', + WebkitLineClamp: Math.floor(maxHeight / TABLE.LINE_HEIGHT), + }), }); export const getJsonCellStyles: TableCellStyles = (_theme, { textWrap, shouldOverflow }) => diff --git a/packages/grafana-ui/src/components/Table/TableNG/TableNG.tsx b/packages/grafana-ui/src/components/Table/TableNG/TableNG.tsx index 75724676a4f..33f0a8f3129 100644 --- a/packages/grafana-ui/src/components/Table/TableNG/TableNG.tsx +++ b/packages/grafana-ui/src/components/Table/TableNG/TableNG.tsx @@ -89,7 +89,6 @@ import { getDisplayName, getIsNestedTable, getJustifyContent, - getMaxHeight, getVisibleFields, isCellInspectEnabled, predicateByName, @@ -113,6 +112,7 @@ export function TableNG(props: TableNGProps) { getActions = () => [], height, initialSortBy, + maxRowHeight: _maxRowHeight, noHeader, onCellFilterAdded, onColumnResize, @@ -203,6 +203,8 @@ export function TableNG(props: TableNGProps) { showTypeIcons: showTypeIcons ?? false, typographyCtx, }); + // the minimum max row height we should honor is a single line of text. + const maxRowHeight = _maxRowHeight != null ? Math.max(TABLE.LINE_HEIGHT, _maxRowHeight) : undefined; const rowHeight = useRowHeight({ columnWidths: widths, fields: visibleFields, @@ -210,6 +212,7 @@ export function TableNG(props: TableNGProps) { defaultHeight: defaultRowHeight, expandedRows, typographyCtx, + maxHeight: maxRowHeight, }); const { @@ -419,8 +422,12 @@ export function TableNG(props: TableNGProps) { const textWrap = rowHeight === 'auto' || shouldTextWrap(field); const withTooltip = withDataLinksActionsTooltip(field, cellType); const canBeColorized = canFieldBeColorized(cellType, applyToRowBgFn); - const maxHeight = getMaxHeight(field); - const cellStyleOptions: TableCellStyleOptions = { textAlign, textWrap, shouldOverflow, maxHeight }; + const cellStyleOptions: TableCellStyleOptions = { + textAlign, + textWrap, + shouldOverflow, + maxHeight: maxRowHeight, + }; result.colsWithTooltip[displayName] = withTooltip; @@ -460,7 +467,7 @@ export function TableNG(props: TableNGProps) { ); @@ -518,8 +525,8 @@ export function TableNG(props: TableNGProps) { ); - if (maxHeight != null) { - result = clampByMaxHeight(maxHeight, result, cellParentStyles); + if (maxRowHeight != null) { + result = clampByMaxHeight(maxRowHeight, result, cellParentStyles); } return result; @@ -534,13 +541,12 @@ export function TableNG(props: TableNGProps) { if (tooltipField) { const tooltipDisplayName = getDisplayName(tooltipField); const tooltipCellOptions = getCellOptions(tooltipField); - const tooltipMaxHeight = getMaxHeight(tooltipField); const tooltipCellStyleOptions = { textAlign: getAlignment(tooltipField), textWrap: shouldTextWrap(tooltipField), shouldOverflow: false, - maxHeight: tooltipMaxHeight, + maxHeight: maxRowHeight, } satisfies TableCellStyleOptions; const tooltipCanBeColorized = canFieldBeColorized(tooltipCellOptions.type, applyToRowBgFn); const tooltipDefaultStyles = getDefaultCellStyles(theme, tooltipCellStyleOptions); @@ -555,10 +561,10 @@ export function TableNG(props: TableNGProps) { const tooltipBodyClasses = clsx(tooltipDefaultStyles, tooltipSpecificStyles, tooltipLinkStyles); let tooltipFieldRenderer = getCellRenderer(tooltipField, tooltipCellOptions); - if (tooltipMaxHeight) { + if (maxRowHeight != null) { const OrigCellRenderer = tooltipFieldRenderer; tooltipFieldRenderer = (props) => - clampByMaxHeight(tooltipMaxHeight, , tooltipBodyClasses); + clampByMaxHeight(maxRowHeight, , tooltipBodyClasses); } const placement = field.config.custom?.tooltip?.placement ?? TableCellTooltipPlacement.Auto; @@ -570,7 +576,7 @@ export function TableNG(props: TableNGProps) { const tooltipProps = { cellOptions: tooltipCellOptions, classes: tooltipClasses, - className: clsx(tooltipClasses.tooltipContent, { [tooltipBodyClasses]: tooltipMaxHeight == null }), + className: clsx(tooltipClasses.tooltipContent, { [tooltipBodyClasses]: maxRowHeight == null }), data, disableSanitizeHtml, field: tooltipField, @@ -664,6 +670,7 @@ export function TableNG(props: TableNGProps) { getCellColorInlineStyles, getTextColorForBackground, isCountRowsSet, + maxRowHeight, numFrozenColsFullyInView, onCellFilterAdded, rowHeight, diff --git a/packages/grafana-ui/src/components/Table/TableNG/hooks.ts b/packages/grafana-ui/src/components/Table/TableNG/hooks.ts index 7a1be693d8c..a7164ef9c70 100644 --- a/packages/grafana-ui/src/components/Table/TableNG/hooks.ts +++ b/packages/grafana-ui/src/components/Table/TableNG/hooks.ts @@ -403,6 +403,7 @@ interface UseRowHeightOptions { defaultHeight: NonNullable; expandedRows: Set; typographyCtx: TypographyCtx; + maxHeight?: number; } export function useRowHeight({ @@ -412,8 +413,12 @@ export function useRowHeight({ defaultHeight, expandedRows, typographyCtx, + maxHeight, }: UseRowHeightOptions): NonNullable | ((row: TableRow) => number) { - const measurers = useMemo(() => buildCellHeightMeasurers(fields, typographyCtx), [fields, typographyCtx]); + const measurers = useMemo( + () => buildCellHeightMeasurers(fields, typographyCtx, maxHeight), + [fields, typographyCtx, maxHeight] + ); const hasWrappedCols = useMemo(() => measurers?.length ?? 0 > 0, [measurers]); const colWidths = useMemo(() => { diff --git a/packages/grafana-ui/src/components/Table/TableNG/types.ts b/packages/grafana-ui/src/components/Table/TableNG/types.ts index 10e419121b2..608edfadb4d 100644 --- a/packages/grafana-ui/src/components/Table/TableNG/types.ts +++ b/packages/grafana-ui/src/components/Table/TableNG/types.ts @@ -132,6 +132,7 @@ export interface BaseTableProps { frozenColumns?: number; enablePagination?: boolean; cellHeight?: TableCellHeight; + maxRowHeight?: number; structureRev?: number; transparent?: boolean; /** @alpha Used by SparklineCell when provided */ diff --git a/packages/grafana-ui/src/components/Table/TableNG/utils.ts b/packages/grafana-ui/src/components/Table/TableNG/utils.ts index e22104dbd6f..cf21a865de8 100644 --- a/packages/grafana-ui/src/components/Table/TableNG/utils.ts +++ b/packages/grafana-ui/src/components/Table/TableNG/utils.ts @@ -84,18 +84,13 @@ export function shouldTextWrap(field: Field): boolean { return Boolean(field.config.custom?.wrapText); } -export function getMaxHeight(field: Field): number | undefined { - return field.config?.custom?.maxHeight; -} - /** * @internal wrap a cell height measurer to clamp its output to the maxHeight defined in the field, if any. */ -function clampByMaxHeight(measurer: MeasureCellHeight): MeasureCellHeight { +function clampByMaxHeight(measurer: MeasureCellHeight, maxHeight?: number): MeasureCellHeight { return (value, width, field, rowIdx, lineHeight) => { const rawHeight = measurer(value, width, field, rowIdx, lineHeight); - const maxHeight = getMaxHeight(field); - if (typeof maxHeight !== 'number') { + if (maxHeight == null) { return rawHeight; } @@ -268,7 +263,8 @@ const spaceRegex = /[\s-]/; */ export function buildCellHeightMeasurers( fields: Field[], - typographyCtx: TypographyCtx + typographyCtx: TypographyCtx, + maxHeight?: number ): MeasureCellHeightEntry[] | undefined { const result: Record = {}; let wrappedFields = 0; @@ -298,8 +294,8 @@ export function buildCellHeightMeasurers( if (!result[measurerFactoryKey]) { const [measure, estimate] = measurerFactory[measurerFactoryKey](); result[measurerFactoryKey] = { - measure: clampByMaxHeight(measure), - estimate: estimate != null ? clampByMaxHeight(estimate) : undefined, + measure: clampByMaxHeight(measure, maxHeight), + estimate: estimate != null ? clampByMaxHeight(estimate, maxHeight) : undefined, fieldIdxs: [], }; } diff --git a/public/app/plugins/panel/table/TablePanel.tsx b/public/app/plugins/panel/table/TablePanel.tsx index bf45bed58cd..850158055e4 100644 --- a/public/app/plugins/panel/table/TablePanel.tsx +++ b/public/app/plugins/panel/table/TablePanel.tsx @@ -80,6 +80,7 @@ export function TablePanel(props: Props) { frozenColumns={options.frozenColumns?.left} enablePagination={options.footer?.enablePagination} cellHeight={options.cellHeight} + maxRowHeight={options.maxRowHeight} timeRange={timeRange} enableSharedCrosshair={config.featureToggles.tableSharedCrosshair && enableSharedCrosshair} fieldConfig={fieldConfig} diff --git a/public/app/plugins/panel/table/module.tsx b/public/app/plugins/panel/table/module.tsx index 83399bbf018..41e4aabbefe 100644 --- a/public/app/plugins/panel/table/module.tsx +++ b/public/app/plugins/panel/table/module.tsx @@ -113,16 +113,6 @@ export const plugin = new PanelPlugin(TablePanel) name: t('table.name-wrap-text', 'Wrap text'), category, }) - .addNumberInput({ - path: 'maxHeight', - name: t('table.text-wrap-options.label-max-height', 'Max cell height'), - category, - settings: { - placeholder: t('table.text-wrap-options.placeholder-max-height', 'none'), - min: 0, - }, - showIf: (cfg) => cfg.wrapText, - }) .addBooleanSwitch({ path: 'wrapHeaderText', name: t('table.name-wrap-header-text', 'Wrap header text'), @@ -208,6 +198,15 @@ export const plugin = new PanelPlugin(TablePanel) ], }, }) + .addNumberInput({ + path: 'maxRowHeight', + name: t('table.text-wrap-options.label-max-height', 'Max row height'), + category, + settings: { + placeholder: t('table.text-wrap-options.placeholder-max-height', 'none'), + min: 0, + }, + }) .addBooleanSwitch({ path: 'footer.show', category: footerCategory, diff --git a/public/app/plugins/panel/table/panelcfg.cue b/public/app/plugins/panel/table/panelcfg.cue index 1bfbd053cf2..cfd699cd727 100644 --- a/public/app/plugins/panel/table/panelcfg.cue +++ b/public/app/plugins/panel/table/panelcfg.cue @@ -44,10 +44,12 @@ composableKinds: PanelCfg: { } // Controls the height of the rows cellHeight?: ui.TableCellHeight & (*"sm" | _) - // Defines the number of columns to freeze on the left side of the table - frozenColumns?: { - left?: number | *0 - } + // limits the maximum height of a row, if text wrapping or dynamic height is enabled + maxRowHeight?: number + // Defines the number of columns to freeze on the left side of the table + frozenColumns?: { + left?: number | *0 + } } @cuetsy(kind="interface") FieldConfig: { ui.TableFieldOptions diff --git a/public/app/plugins/panel/table/panelcfg.gen.ts b/public/app/plugins/panel/table/panelcfg.gen.ts index dba9153ea0c..db1994b1c42 100644 --- a/public/app/plugins/panel/table/panelcfg.gen.ts +++ b/public/app/plugins/panel/table/panelcfg.gen.ts @@ -29,6 +29,10 @@ export interface Options { frozenColumns?: { left?: number; }; + /** + * limits the maximum height of a row, if text wrapping or dynamic height is enabled + */ + maxRowHeight?: number; /** * Controls whether the panel should show the header */ diff --git a/public/locales/en-US/grafana.json b/public/locales/en-US/grafana.json index 14c4abc0315..bdf1926cbd0 100644 --- a/public/locales/en-US/grafana.json +++ b/public/locales/en-US/grafana.json @@ -12809,7 +12809,7 @@ "placeholder-column-width": "auto", "placeholder-fields": "All Numeric Fields", "text-wrap-options": { - "label-max-height": "Max cell height", + "label-max-height": "Max row height", "placeholder-max-height": "none" }, "tooltip-placement-options": {