diff --git a/packages/table/src/common/grid.ts b/packages/table/src/common/grid.ts index eb2dde20fa..76ad447e93 100644 --- a/packages/table/src/common/grid.ts +++ b/packages/table/src/common/grid.ts @@ -49,6 +49,12 @@ export class Grid { public static DEFAULT_GHOST_WIDTH = 150; + // defined in headers/_common.scss + public static MIN_COLUMN_HEADER_HEIGHT = 30; + + // defined in headers/_common.scss + public static MIN_ROW_HEADER_WIDTH = 30; + public numCols: number; public numRows: number; diff --git a/packages/table/src/headers/columnHeader.tsx b/packages/table/src/headers/columnHeader.tsx index 298b5935b4..4932847b28 100644 --- a/packages/table/src/headers/columnHeader.tsx +++ b/packages/table/src/headers/columnHeader.tsx @@ -88,8 +88,6 @@ export class ColumnHeader extends React.Component { } = this.props; return ( - // HACKHACK(adahiya): strange shouldComponentUpdate type error with strict null checks - // @ts-ignore
{ } = this.props; return ( - // HACKHACK(adahiya): strange shouldComponentUpdate type error with strict null checks - // @ts-ignore
viewportRect.height - columnHeaderHeight; + } + + /** + * Pass in an already-computed viewport rect here, if available, to reduce DOM reads. + * + * @returns whether the rendered columns overflow the visible viewport horizontally, helpful for scrolling calculations + */ + public hasHorizontalOverflow(rowHeaderWidth = Grid.MIN_ROW_HEADER_WIDTH, viewportRect = this.getViewportRect()) { + if (this.grid === undefined) { + return false; + } + return this.grid.getWidth() > viewportRect.width - rowHeaderWidth; + } + // Converters // ========== diff --git a/packages/table/src/table2.tsx b/packages/table/src/table2.tsx index 9e4e2f2a4b..514e66279a 100644 --- a/packages/table/src/table2.tsx +++ b/packages/table/src/table2.tsx @@ -207,10 +207,20 @@ export class Table2 extends AbstractComponent2 (this.cellContainerElement = ref), - columnHeader: (ref: HTMLElement | null) => (this.columnHeaderElement = ref), + columnHeader: (ref: HTMLElement | null) => { + this.columnHeaderElement = ref; + if (ref != null) { + this.columnHeaderHeight = ref.clientHeight; + } + }, quadrantStack: (ref: TableQuadrantStack) => (this.quadrantStackInstance = ref), rootTable: (ref: HTMLElement | null) => (this.rootTableElement = ref), - rowHeader: (ref: HTMLElement | null) => (this.rowHeaderElement = ref), + rowHeader: (ref: HTMLElement | null) => { + this.rowHeaderElement = ref; + if (ref != null) { + this.rowHeaderWidth = ref.clientWidth; + } + }, scrollContainer: (ref: HTMLElement | null) => (this.scrollContainerElement = ref), }; @@ -218,12 +228,16 @@ export class Table2 extends AbstractComponent2; + } + + // if we have horizontal overflow, no need to render ghost columns + // (this avoids problems like https://github.com/palantir/blueprint/issues/5027) + const hasHorizontalOverflow = this.locator.hasHorizontalOverflow(this.rowHeaderWidth, viewportRect); + const columnIndices = this.grid.getColumnIndicesInRect( + viewportRect, + hasHorizontalOverflow ? false : enableGhostCells, + ); + const columnIndexStart = showFrozenColumnsOnly ? 0 : columnIndices.columnIndexStart; const columnIndexEnd = showFrozenColumnsOnly ? this.getMaxFrozenColumnIndex() : columnIndices.columnIndexEnd; return ( -
+
; + } + + // if we have vertical overflow, no need to render ghost rows + // (this avoids problems like https://github.com/palantir/blueprint/issues/5027) + const hasVerticalOverflow = this.locator.hasVerticalOverflow(this.columnHeaderHeight, viewportRect); + const rowIndices = this.grid.getRowIndicesInRect(viewportRect, hasVerticalOverflow ? false : enableGhostCells); + const rowIndexStart = showFrozenRowsOnly ? 0 : rowIndices.rowIndexStart; const rowIndexEnd = showFrozenRowsOnly ? this.getMaxFrozenRowIndex() : rowIndices.rowIndexEnd; @@ -926,8 +954,15 @@ export class Table2 extends AbstractComponent2) => { - // Prevent the event from propagating to avoid a resize event on the - // resize sensor. + // Prevent the event from propagating to avoid a resize event on the resize sensor. event.stopPropagation(); if (this.locator != null && !this.state.isLayoutLocked) { - const viewportRect = this.locator.getViewportRect(); - this.updateViewportRect(viewportRect); + const newViewportRect = this.locator.getViewportRect(); + this.updateViewportRect(newViewportRect); } }; diff --git a/packages/table/src/tableBody.tsx b/packages/table/src/tableBody.tsx index 85baea4b3b..98bf25854f 100644 --- a/packages/table/src/tableBody.tsx +++ b/packages/table/src/tableBody.tsx @@ -62,8 +62,9 @@ export class TableBody extends AbstractComponent2 { renderMode: RenderMode.BATCH, }; - // TODO: Does this method need to be public? - // (see: https://github.com/palantir/blueprint/issues/1617) + /** + * @deprecated, will be removed from public API in the next major version + */ public static cellClassNames(rowIndex: number, columnIndex: number) { return cellClassNames(rowIndex, columnIndex); } diff --git a/packages/table/test/table2Tests.tsx b/packages/table/test/table2Tests.tsx index d5ea252127..c7f7773a22 100644 --- a/packages/table/test/table2Tests.tsx +++ b/packages/table/test/table2Tests.tsx @@ -31,6 +31,7 @@ import type { ColumnIndices, RowIndices } from "../src/common/grid"; import { Rect } from "../src/common/rect"; import { RenderMode } from "../src/common/renderMode"; import { TableQuadrant } from "../src/quadrants/tableQuadrant"; +import { TableQuadrantStack } from "../src/quadrants/tableQuadrantStack"; import { IRegion, Regions } from "../src/regions"; import { TableState } from "../src/tableState"; import { CellType, expectCellLoading } from "./cellTestUtils"; @@ -208,12 +209,35 @@ describe("", function (this) { }); }); - function mountTable(tableProps: Partial = {}) { + it("does not render ghost columns when there is horizontal overflow", () => { + const { containerElement } = mountTable( + { numRows: 2, defaultRowHeight: 20, defaultColumnWidth: 100 }, + { + height: 200, + // 300px leaves just enough space for the 3 columns, but there is 30px taken up by + // the row header, which will overflow. + width: 300, + }, + ); + const numGhostCellsInFirstRow = containerElement.querySelectorAll( + `.${Classes.TABLE_CELL_GHOST}.${Classes.rowCellIndexClass(0)}`, + ).length; + expect(numGhostCellsInFirstRow).to.be.eq(0); + + // cleanup + document.body.removeChild(containerElement); + }); + + function mountTable( + tableProps: Partial = {}, + tableDimensions: { width: number; height: number } = { width: CONTAINER_WIDTH, height: CONTAINER_HEIGHT }, + ) { const containerElement = document.createElement("div"); - containerElement.style.width = `${CONTAINER_WIDTH}px`; - containerElement.style.height = `${CONTAINER_HEIGHT}px`; + containerElement.style.width = `${tableDimensions.width}px`; + containerElement.style.height = `${tableDimensions.height}px`; document.body.appendChild(containerElement); + TableQuadrantStack.defaultProps.throttleScrolling = false; const table = mount( @@ -226,6 +250,65 @@ describe("", function (this) { } }); + describe("Vertically scrolling", () => { + runTestToEnsureScrollingIsEnabled(true); + runTestToEnsureScrollingIsEnabled(false); + + it("does not render ghost rows when there is vertical overflow", () => { + const { containerElement } = mountTable( + { defaultRowHeight: 20, enableGhostCells: true }, + { + // we need _some_ amount of vertical overflow to avoid the code path which disables vertical scroll + // in the table altogether. 200px leaves just enough space for the rows, but there is 30px taken up by + // the column header, which will overflow. + height: 200, + width: 300, + }, + ); + const numGhostCellsInFirstColumn = containerElement.querySelectorAll( + `.${Classes.TABLE_CELL_GHOST}.${Classes.columnCellIndexClass(0)}`, + ).length; + expect(numGhostCellsInFirstColumn).to.be.eq(0); + + // cleanup + document.body.removeChild(containerElement); + }); + + function runTestToEnsureScrollingIsEnabled(enableGhostCells: boolean) { + it(`isn't disabled when there is half a row left to scroll to and enableGhostCells is set to ${enableGhostCells}`, () => { + const { containerElement, table } = mountTable( + { defaultRowHeight: 30, enableGhostCells }, + { + height: 320, + width: 300, + }, + ); + const tableContainer = table.find(`.${Classes.TABLE_CONTAINER}`); + // There should be 10px left of scrolling. Height is 320, rows take up 300, and headerRow takes up 30 + expect(tableContainer.hasClass(Classes.TABLE_NO_VERTICAL_SCROLL)).to.be.false; + + // clean up created div + document.body.removeChild(containerElement); + }); + } + + function mountTable(tableProps: Partial = {}, tableDimensions: { width: number; height: number }) { + const containerElement = document.createElement("div"); + containerElement.style.width = `${tableDimensions.width}px`; + containerElement.style.height = `${tableDimensions.height}px`; + document.body.appendChild(containerElement); + + TableQuadrantStack.defaultProps.throttleScrolling = false; + const table = mount( + + + , + { attachTo: containerElement }, + ); + return { containerElement, table }; + } + }); + describe("Instance methods", () => { describe("resizeRowsByApproximateHeight", () => { const STR_LENGTH_SHORT = 10; diff --git a/packages/table/test/tableBodyTests.tsx b/packages/table/test/tableBodyTests.tsx index 3224812773..d3b0cddee1 100644 --- a/packages/table/test/tableBodyTests.tsx +++ b/packages/table/test/tableBodyTests.tsx @@ -28,6 +28,7 @@ import { RenderMode } from "../src/common/renderMode"; import { MenuContext } from "../src/interactions/menus/menuContext"; import { IRegion, Regions } from "../src/regions"; import { ITableBodyProps, TableBody } from "../src/tableBody"; +import { cellClassNames } from "../src/tableBodyCells"; describe("TableBody", () => { // use enough rows that batching won't render all of them in one pass. @@ -40,11 +41,8 @@ describe("TableBody", () => { const ROW_HEIGHT = 20; it("cellClassNames", () => { - expect(TableBody.cellClassNames(0, 0)).to.deep.equal([ - Classes.rowCellIndexClass(0), - Classes.columnCellIndexClass(0), - ]); - expect(TableBody.cellClassNames(4096, 1024)).to.deep.equal([ + expect(cellClassNames(0, 0)).to.deep.equal([Classes.rowCellIndexClass(0), Classes.columnCellIndexClass(0)]); + expect(cellClassNames(4096, 1024)).to.deep.equal([ Classes.rowCellIndexClass(4096), Classes.columnCellIndexClass(1024), ]);