From 3dd948d1007542fd249b829fa6e81b80fc81f4a9 Mon Sep 17 00:00:00 2001 From: Chris Lewis Date: Wed, 16 Aug 2017 11:56:44 -0700 Subject: [PATCH 01/18] Get rid of unneeded TABLE_DRAGGABLE class --- packages/table/src/common/classes.ts | 1 - packages/table/src/headers/columnHeader.tsx | 4 +--- packages/table/src/headers/header.tsx | 3 +-- 3 files changed, 2 insertions(+), 6 deletions(-) diff --git a/packages/table/src/common/classes.ts b/packages/table/src/common/classes.ts index 9c81c563f8..8224d39bc9 100644 --- a/packages/table/src/common/classes.ts +++ b/packages/table/src/common/classes.ts @@ -20,7 +20,6 @@ export const TABLE_COLUMN_HEADERS = "bp-table-column-headers"; export const TABLE_COLUMN_NAME = "bp-table-column-name"; export const TABLE_COLUMN_NAME_TEXT = "bp-table-column-name-text"; export const TABLE_CONTAINER = "bp-table-container"; -export const TABLE_DRAGGABLE = "bp-table-draggable"; export const TABLE_DRAGGING = "bp-table-dragging"; export const TABLE_EDITABLE_NAME = "bp-table-editable-name"; export const TABLE_FOCUS_REGION = "bp-table-focus-region"; diff --git a/packages/table/src/headers/columnHeader.tsx b/packages/table/src/headers/columnHeader.tsx index c47afbc412..5ca5c74b02 100644 --- a/packages/table/src/headers/columnHeader.tsx +++ b/packages/table/src/headers/columnHeader.tsx @@ -116,9 +116,7 @@ private wrapCells = (cells: Array>) => { width: tableWidth - scrollLeftCorrection, }; - const classes = classNames(Classes.TABLE_THEAD, Classes.TABLE_COLUMN_HEADER_TR, { - [Classes.TABLE_DRAGGABLE] : (this.props.onSelection != null), - }); + const classes = classNames(Classes.TABLE_THEAD, Classes.TABLE_COLUMN_HEADER_TR); // add a wrapper set to the full-table width to ensure container styles stretch from the first // cell all the way to the last diff --git a/packages/table/src/headers/header.tsx b/packages/table/src/headers/header.tsx index d32b700bc5..2c1e387d21 100644 --- a/packages/table/src/headers/header.tsx +++ b/packages/table/src/headers/header.tsx @@ -303,7 +303,7 @@ export class Header extends React.Component } private renderCell = (index: number, extremaClasses: string[]) => { - const { getIndexClass, onSelection, selectedRegions } = this.props; + const { getIndexClass, selectedRegions } = this.props; const cell = this.props.renderHeaderCell(index); @@ -312,7 +312,6 @@ export class Header extends React.Component const isEntireCellTargetReorderable = this.isEntireCellTargetReorderable(cell, isSelected); const className = classNames(extremaClasses, { - [Classes.TABLE_DRAGGABLE]: onSelection != null, [Classes.TABLE_HEADER_REORDERABLE]: isEntireCellTargetReorderable, }, this.props.getCellIndexClass(index), cell.props.className); From df1ac0b195b312cad84867cc35ddb04e18468da8 Mon Sep 17 00:00:00 2001 From: Chris Lewis Date: Wed, 16 Aug 2017 12:13:01 -0700 Subject: [PATCH 02/18] Always show reorder handle if column header cell is reorderable --- .../table/src/headers/columnHeaderCell.tsx | 5 +++-- packages/table/src/headers/header.tsx | 18 +++++++++--------- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/packages/table/src/headers/columnHeaderCell.tsx b/packages/table/src/headers/columnHeaderCell.tsx index bf08586064..36c5691870 100644 --- a/packages/table/src/headers/columnHeaderCell.tsx +++ b/packages/table/src/headers/columnHeaderCell.tsx @@ -146,7 +146,7 @@ export class ColumnHeaderCell extends AbstractComponent{name}; @@ -161,7 +161,7 @@ export class ColumnHeaderCell extends AbstractComponent
- {this.props.reorderHandle} + {reorderHandle} {dropdownMenu}
@@ -171,6 +171,7 @@ export class ColumnHeaderCell extends AbstractComponent + {reorderHandle} {dropdownMenu}
{nameComponent}
diff --git a/packages/table/src/headers/header.tsx b/packages/table/src/headers/header.tsx index 2c1e387d21..a0d6284dba 100644 --- a/packages/table/src/headers/header.tsx +++ b/packages/table/src/headers/header.tsx @@ -309,7 +309,7 @@ export class Header extends React.Component const isLoading = cell.props.loading != null ? cell.props.loading : this.props.loading; const isSelected = this.props.isCellSelected(index); - const isEntireCellTargetReorderable = this.isEntireCellTargetReorderable(cell, isSelected); + const isEntireCellTargetReorderable = this.isEntireCellTargetReorderable(isSelected); const className = classNames(extremaClasses, { [Classes.TABLE_HEADER_REORDERABLE]: isEntireCellTargetReorderable, @@ -321,7 +321,7 @@ export class Header extends React.Component [this.props.headerCellIsSelectedPropName]: isSelected, [this.props.headerCellIsReorderablePropName]: isEntireCellTargetReorderable, loading: isLoading, - reorderHandle: this.maybeRenderReorderHandle(cell, index), + reorderHandle: this.maybeRenderReorderHandle(index), }; const modifiedHandleSizeChanged = (size: number) => this.props.handleSizeChanged(index, size); @@ -358,18 +358,18 @@ export class Header extends React.Component ); - return this.isReorderHandleEnabled(cell) + return this.isReorderHandleEnabled() ? baseChildren // reordering will be handled by interacting with the reorder handle : this.wrapInDragReorderable(index, baseChildren, !isEntireCellTargetReorderable); } - private isReorderHandleEnabled(cell: JSX.Element) { + private isReorderHandleEnabled() { // the reorder handle can only appear in the column interaction bar - return this.isColumnHeader() && cell.props.useInteractionBar && this.props.isReorderable; + return this.isColumnHeader() && this.props.isReorderable; } - private maybeRenderReorderHandle(cell: JSX.Element, index: number) { - return !this.isReorderHandleEnabled(cell) + private maybeRenderReorderHandle(index: number) { + return !this.isReorderHandleEnabled() ? undefined : this.wrapInDragReorderable(index,
@@ -411,7 +411,7 @@ export class Header extends React.Component this.setState({ hasSelectionEnded: true }); } - private isEntireCellTargetReorderable = (cell: JSX.Element, isSelected: boolean) => { + private isEntireCellTargetReorderable = (isSelected: boolean) => { const { selectedRegions } = this.props; // although reordering may be generally enabled for this row/column (via props.isReorderable), the // row/column shouldn't actually become reorderable from a user perspective until a few other @@ -429,7 +429,7 @@ export class Header extends React.Component // both selection and reordering behavior. && selectedRegions.length === 1 // columns are reordered via a reorder handle, so drag-selection needn't be disabled - && !this.isReorderHandleEnabled(cell); + && !this.isReorderHandleEnabled(); } } From 1da228830fe58d065dcafe7aa655de740742e21f Mon Sep 17 00:00:00 2001 From: Chris Lewis Date: Wed, 16 Aug 2017 12:13:19 -0700 Subject: [PATCH 03/18] Add new state classes --- packages/table/src/common/classes.ts | 3 +++ packages/table/src/headers/columnHeaderCell.tsx | 6 ++++++ 2 files changed, 9 insertions(+) diff --git a/packages/table/src/common/classes.ts b/packages/table/src/common/classes.ts index 8224d39bc9..9ce62da655 100644 --- a/packages/table/src/common/classes.ts +++ b/packages/table/src/common/classes.ts @@ -17,6 +17,9 @@ export const TABLE_CELL_LEDGER_EVEN = "bp-table-cell-ledger-even"; export const TABLE_CELL_LEDGER_ODD = "bp-table-cell-ledger-odd"; export const TABLE_COLUMN_HEADER_TR = "bp-table-column-header-tr"; export const TABLE_COLUMN_HEADERS = "bp-table-column-headers"; +export const TABLE_COLUMN_HEADER_CELL = "bp-table-column-header-cell"; +export const TABLE_COLUMN_HEADER_CELL_HAS_REORDER_HANDLE = "bp-table-column-header-cell-has-reorder-handle"; +export const TABLE_COLUMN_HEADER_CELL_HAS_INTERACTION_BAR = "bp-table-column-header-cell-has-interaction-bar"; export const TABLE_COLUMN_NAME = "bp-table-column-name"; export const TABLE_COLUMN_NAME_TEXT = "bp-table-column-name-text"; export const TABLE_CONTAINER = "bp-table-container"; diff --git a/packages/table/src/headers/columnHeaderCell.tsx b/packages/table/src/headers/columnHeaderCell.tsx index 36c5691870..b747a224c7 100644 --- a/packages/table/src/headers/columnHeaderCell.tsx +++ b/packages/table/src/headers/columnHeaderCell.tsx @@ -123,11 +123,17 @@ export class ColumnHeaderCell extends AbstractComponent {this.renderName()} {this.maybeRenderContent()} From 9b93cc4e722d47e552cbd044443754a92a52d7ea Mon Sep 17 00:00:00 2001 From: Chris Lewis Date: Wed, 16 Aug 2017 12:13:36 -0700 Subject: [PATCH 04/18] Enable column reordering in example --- packages/table/preview/index.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/table/preview/index.tsx b/packages/table/preview/index.tsx index aa98767c4b..ba821f17c9 100644 --- a/packages/table/preview/index.tsx +++ b/packages/table/preview/index.tsx @@ -163,7 +163,7 @@ class MutableTable extends React.Component<{}, IMutableTableState> { enableCellSelection: true, enableCellTruncation: false, enableColumnNameEditing: false, - enableColumnReordering: false, + enableColumnReordering: true, enableColumnResizing: false, enableColumnSelection: true, enableContextMenu: false, From ead53f951527512a82f8a91ed68d903482da2f42 Mon Sep 17 00:00:00 2001 From: Chris Lewis Date: Wed, 16 Aug 2017 12:36:26 -0700 Subject: [PATCH 05/18] Style the reorder handle --- .../table/src/interactions/_interactions.scss | 21 ++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/packages/table/src/interactions/_interactions.scss b/packages/table/src/interactions/_interactions.scss index e47e026e07..3cd418ac6d 100644 --- a/packages/table/src/interactions/_interactions.scss +++ b/packages/table/src/interactions/_interactions.scss @@ -9,6 +9,8 @@ $resize-handle-padding: 2px !default; $resize-handle-color: $pt-intent-primary !default; $resize-handle-dragging-color: $pt-intent-primary !default; +$reorder-handle-width: 15px !default; + @mixin grabbable() { cursor: grab; @@ -48,11 +50,25 @@ $resize-handle-dragging-color: $pt-intent-primary !default; } } +.bp-table-column-header-cell { + // break up this selector to reduce line length + &.bp-table-column-header-cell-has-reorder-handle { + &:not(.bp-table-column-header-cell-has-interaction-bar) .bp-table-column-name-text { + padding-left: $reorder-handle-width; + } + } +} + .bp-table-reorder-handle-target { @include grabbable(); position: absolute; top: 0; left: 0; + bottom: 0; + display: flex; + align-items: center; + justify-content: center; + width: $reorder-handle-width; color: $gray3; &:hover { @@ -62,11 +78,6 @@ $resize-handle-dragging-color: $pt-intent-primary !default; &:active { color: $pt-text-color; } - - .bp-table-reorder-handle { - padding: 3px 6px 3px 1px; - line-height: 14px; - } } // The wide, transparent, clickable target that contains the From b32bdedd7d41d47c4682bc17736a0c722913e04f Mon Sep 17 00:00:00 2001 From: Chris Lewis Date: Wed, 16 Aug 2017 14:43:19 -0700 Subject: [PATCH 06/18] Newly reordered region becomes the only selection --- packages/table/src/interactions/reorderable.tsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/table/src/interactions/reorderable.tsx b/packages/table/src/interactions/reorderable.tsx index 2907bcbded..e26224a422 100644 --- a/packages/table/src/interactions/reorderable.tsx +++ b/packages/table/src/interactions/reorderable.tsx @@ -155,8 +155,9 @@ export class DragReorderable extends React.Component { const reorderedIndex = Utils.guideIndexToReorderedIndex(oldIndex, guideIndex, length); this.props.onReordered(oldIndex, reorderedIndex, length); + // the newly reordered region becomes the only selection const newRegion = this.props.toRegion(reorderedIndex, reorderedIndex + length - 1); - this.props.onSelection(Regions.update(this.props.selectedRegions, newRegion)); + this.props.onSelection(Regions.update([], newRegion)); // resetting is not strictly required, but it's cleaner this.selectedRegionStartIndex = undefined; From a0b269bfc2bdbd3403af0de3d04cb51c1d0f9b7e Mon Sep 17 00:00:00 2001 From: Chris Lewis Date: Wed, 16 Aug 2017 14:57:29 -0700 Subject: [PATCH 07/18] Fix reordering in example when selection disabled --- packages/table/preview/index.tsx | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/table/preview/index.tsx b/packages/table/preview/index.tsx index ba821f17c9..cb1d59f5df 100644 --- a/packages/table/preview/index.tsx +++ b/packages/table/preview/index.tsx @@ -637,11 +637,13 @@ class MutableTable extends React.Component<{}, IMutableTableState> { private onColumnsReordered = (oldIndex: number, newIndex: number, length: number) => { this.maybeLogCallback(`[onColumnsReordered] oldIndex = ${oldIndex} newIndex = ${newIndex} length = ${length}`); this.store.reorderColumns(oldIndex, newIndex, length); + this.forceUpdate(); } private onRowsReordered = (oldIndex: number, newIndex: number, length: number) => { this.maybeLogCallback(`[onRowsReordered] oldIndex = ${oldIndex} newIndex = ${newIndex} length = ${length}`); this.store.reorderRows(oldIndex, newIndex, length); + this.forceUpdate(); } private onColumnWidthChanged = (index: number, size: number) => { From 5e096b05826410e58da475f2e52b2af36d6147e6 Mon Sep 17 00:00:00 2001 From: Chris Lewis Date: Wed, 16 Aug 2017 15:09:22 -0700 Subject: [PATCH 08/18] Fix editable-cell border in column header --- packages/table/src/interactions/_interactions.scss | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/packages/table/src/interactions/_interactions.scss b/packages/table/src/interactions/_interactions.scss index 3cd418ac6d..bd8484f5aa 100644 --- a/packages/table/src/interactions/_interactions.scss +++ b/packages/table/src/interactions/_interactions.scss @@ -50,12 +50,19 @@ $reorder-handle-width: 15px !default; } } +// break up this selector to reduce line length .bp-table-column-header-cell { - // break up this selector to reduce line length - &.bp-table-column-header-cell-has-reorder-handle { - &:not(.bp-table-column-header-cell-has-interaction-bar) .bp-table-column-name-text { + &.bp-table-column-header-cell-has-reorder-handle:not(.bp-table-column-header-cell-has-interaction-bar) { + .bp-table-column-name-text { padding-left: $reorder-handle-width; } + + // keep the editable cell input flush with the cell's left border + .bp-table-column-name .bp-table-editable-name.pt-editable-text { + &::before { + left: -$reorder-handle-width; + } + } } } From 471d5c76fc8f2ebc753dc266441f054781e78558 Mon Sep 17 00:00:00 2001 From: Chris Lewis Date: Thu, 17 Aug 2017 09:14:29 -0700 Subject: [PATCH 09/18] Fix reordering tests --- packages/table/src/headers/header.tsx | 2 +- packages/table/test/columnHeaderCellTests.tsx | 12 +- packages/table/test/tableTests.tsx | 128 +++++++----------- 3 files changed, 52 insertions(+), 90 deletions(-) diff --git a/packages/table/src/headers/header.tsx b/packages/table/src/headers/header.tsx index a0d6284dba..bf88ecd28b 100644 --- a/packages/table/src/headers/header.tsx +++ b/packages/table/src/headers/header.tsx @@ -332,7 +332,7 @@ export class Header extends React.Component ", () => { }); describe("Reorder handle", () => { - const REORDER_HANDLE_CLASS = "reorder-handle"; + const REORDER_HANDLE_CLASS = "bp-table-reorder-handle-target"; it("shows reorder handle in interaction bar if reordering and interaction bar are enabled", () => { const element = mount({ useInteractionBar: true, isColumnReorderable: true }); - expect(doesReorderHandleExist(element)).to.be.true; + expect(element.find(`.${Classes.TABLE_INTERACTION_BAR} .${REORDER_HANDLE_CLASS}`).exists()).to.be.true; }); - it("hides reorder handle if reordering enabled but interaction bar disabled", () => { + it("shows reorder handle next to column name if reordering enabled but interaction bar disabled", () => { const element = mount({ useInteractionBar: false, isColumnReorderable: true }); - expect(doesReorderHandleExist(element)).to.be.false; + expect(element.find(`.${Classes.TABLE_COLUMN_NAME} .${REORDER_HANDLE_CLASS}`).exists()).to.be.true; }); - function doesReorderHandleExist(element: ElementHarness) { - return element.find(`.${REORDER_HANDLE_CLASS}`).exists(); - } - function mount(props: Partial & object) { const element = harness.mount( ", () => { const NUM_COLUMNS = 5; const NUM_ROWS = 5; - // table hardcodes itself to 60px tall in Phantom, so use a tiny sizes to ensure - // all rows fit. + const REORDER_HANDLE_WIDTH_IN_PX = 15; + + // table hardcodes itself to 60px tall in Phantom, so use small enough sizes to ensure + // all rows fit. also, ensure the columns are wide enough to fit their reorder handle. const HEIGHT_IN_PX = 5; - const WIDTH_IN_PX = 5; + const WIDTH_IN_PX = 20; - const OFFSET_X = (NEW_INDEX + LENGTH) * WIDTH_IN_PX; + const OFFSET_X = (NEW_INDEX + LENGTH) * WIDTH_IN_PX - REORDER_HANDLE_WIDTH_IN_PX; const OFFSET_Y = (NEW_INDEX + LENGTH) * HEIGHT_IN_PX; - let onColumnsReordered: Sinon.SinonSpy; - let onRowsReordered: Sinon.SinonSpy; - let onSelection: Sinon.SinonSpy; + const onColumnsReordered: Sinon.SinonSpy = sinon.spy(); + const onRowsReordered: Sinon.SinonSpy = sinon.spy(); + const onSelection: Sinon.SinonSpy = sinon.spy(); - beforeEach(() => { - onColumnsReordered = sinon.spy(); - onRowsReordered = sinon.spy(); - onSelection = sinon.spy(); + afterEach(() => { + onColumnsReordered.reset(); + onRowsReordered.reset(); + onSelection.reset(); }); - it("Shows preview guide and invokes callback when columns reordered", () => { + it("Shows preview guide and invokes callback when selected columns reordered", () => { const table = mountTable({ isColumnReorderable: true, onColumnsReordered, selectedRegions: [Regions.column(OLD_INDEX, LENGTH - 1)], }); - const header = getTableHeader(getColumnHeadersWrapper(table), 0); - header.mouse("mousedown").mouse("mousemove", OFFSET_X); + const headerCell = getHeaderCell(getColumnHeadersWrapper(table), 0); + const reorderHandle = getReorderHandle(headerCell); + reorderHandle.mouse("mousedown").mouse("mousemove", OFFSET_X); const guide = table.find(`.${Classes.TABLE_VERTICAL_GUIDE}`); expect(guide).to.exist; - header.mouse("mouseup", OFFSET_X); + reorderHandle.mouse("mouseup", OFFSET_X); expect(onColumnsReordered.called).to.be.true; expect(onColumnsReordered.calledWith(OLD_INDEX, NEW_INDEX, LENGTH)).to.be.true; }); - it("Shows preview guide and invokes callback when rows reordered", () => { + it("Shows preview guide and invokes callback when selected rows reordered", () => { const table = mountTable({ isRowReorderable: true, onRowsReordered, selectedRegions: [Regions.row(OLD_INDEX, LENGTH - 1)], }); - const header = getTableHeader(getRowHeadersWrapper(table), 0); - header.mouse("mousedown").mouse("mousemove", 0, OFFSET_Y); + const headerCell = getHeaderCell(getRowHeadersWrapper(table), 0); + headerCell.mouse("mousedown").mouse("mousemove", 0, OFFSET_Y); const guide = table.find(`.${Classes.TABLE_HORIZONTAL_GUIDE}`); expect(guide).to.exist; - header.mouse("mouseup", 0, OFFSET_Y); + headerCell.mouse("mouseup", 0, OFFSET_Y); expect(onRowsReordered.called).to.be.true; expect(onRowsReordered.calledWith(OLD_INDEX, NEW_INDEX, LENGTH)).to.be.true; }); - it("Doesn't work on columns if there is no selected region defined yet", () => { + it("Reorders an unselected column and selects it afterward", () => { const table = mountTable({ isColumnReorderable: true, onColumnsReordered, + onSelection, }); - getTableHeader(getColumnHeadersWrapper(table), 0) + const headerCell = getHeaderCell(getColumnHeadersWrapper(table), 0); + const reorderHandle = getReorderHandle(headerCell); + reorderHandle .mouse("mousedown") .mouse("mousemove", OFFSET_X) .mouse("mouseup", OFFSET_X); - expect(onColumnsReordered.called).to.be.false; + expect(onColumnsReordered.called).to.be.true; + expect(onSelection.firstCall.calledWith([Regions.column(0)])); }); it("Doesn't work on rows if there is no selected region defined yet", () => { @@ -758,81 +765,36 @@ describe("", () => { isColumnReorderable: true, onColumnsReordered, }); - getTableHeader(getColumnHeadersWrapper(table), 0) + const headerCell = getHeaderCell(getColumnHeadersWrapper(table), 0); + headerCell .mouse("mousedown") .mouse("mousemove", OFFSET_X) .mouse("mouseup", OFFSET_X); expect(onColumnsReordered.called).to.be.false; }); - it("Selecting a column via click and then reordering it works", () => { + it("Clears all selections except the reordered column after reordering", () => { const table = mountTable({ isColumnReorderable: true, onColumnsReordered, onSelection, + selectedRegions: [Regions.column(1)], // some other column }); - const header = getTableHeader(getColumnHeadersWrapper(table), 0); - - // "click" doesn't trigger DragHandler.onActivate - header.mouse("mousedown").mouse("mouseup"); - expect(onSelection.called).to.be.true; + const headerCell = getHeaderCell(getColumnHeadersWrapper(table), 0); + const reorderHandle = getReorderHandle(headerCell); // now we can reorder the column one spot to the right const newIndex = 1; const length = 1; - const offsetX = (newIndex + length) * WIDTH_IN_PX; - header.mouse("mousedown") + const offsetX = (newIndex + length) * WIDTH_IN_PX - REORDER_HANDLE_WIDTH_IN_PX; + reorderHandle.mouse("mousedown") .mouse("mousemove", offsetX) .mouse("mouseup", offsetX); - expect(onColumnsReordered.called).to.be.true; - expect(onColumnsReordered.calledWith(OLD_INDEX, newIndex, length)).to.be.true; - }); - - it("Selecting multiple columns via click+drag and then reordering works", () => { - const table = mountTable({ - isColumnReorderable: true, - onColumnsReordered, - onSelection, - }); - const header = getTableHeader(getColumnHeadersWrapper(table), 0); - const selectionOffsetX = (OLD_INDEX + LENGTH) * WIDTH_IN_PX; - header - .mouse("mousedown") - .mouse("mousemove", selectionOffsetX) - .mouse("mouseup", selectionOffsetX); - expect(onSelection.called).to.be.true; - header.mouse("mousedown") - .mouse("mousemove", OFFSET_X) - .mouse("mouseup", OFFSET_X); - expect(onColumnsReordered.called).to.be.true; - expect(onColumnsReordered.calledWith(OLD_INDEX, NEW_INDEX, LENGTH)).to.be.true; - }); - - it("Moves uncontrolled selection with reordered column when reordering is complete", () => { - const table = mountTable({ - isColumnReorderable: true, - onColumnsReordered, - }); - const headers = getColumnHeadersWrapper(table); - const oldHeader = getTableHeader(headers, 0); - const newHeader = getTableHeader(headers, 1); - - const newIndex = 1; - const length = 1; - const offsetX = (newIndex + length) * WIDTH_IN_PX; - - // select the old header - oldHeader.mouse("mousedown").mouse("mouseup"); - - // show selection region while reordering - oldHeader.mouse("mousedown").mouse("mousemove", offsetX); - expect(table.find(`.${Classes.TABLE_SELECTION_REGION}`).exists()).to.be.true; - - oldHeader.mouse("mouseup", offsetX); - expect(table.find(`.${Classes.TABLE_SELECTION_REGION}`).exists()).to.be.true; - expect(oldHeader.hasClass(Classes.TABLE_HEADER_SELECTED)).to.be.false; - expect(newHeader.hasClass(Classes.TABLE_HEADER_SELECTED)).to.be.true; + // called once on mousedown (to select column 0), once on mouseup (to move the selection) + expect(onSelection.calledTwice).to.be.true; + expect(onSelection.firstCall.calledWith([Regions.column(0)])).to.be.true; + expect(onSelection.secondCall.calledWith([Regions.column(newIndex)])).to.be.true; }); function mountTable(props: Partial) { @@ -861,8 +823,12 @@ describe("
", () => { return table.find(`.${Classes.TABLE_ROW_HEADERS}`); } - function getTableHeader(headersWrapper: ElementHarness, columnIndex: number) { - return headersWrapper.find(`.${Classes.TABLE_HEADER}`, columnIndex); + function getHeaderCell(headersWrapper: ElementHarness, index: number) { + return headersWrapper.find(`.${Classes.TABLE_HEADER}`, index); + } + + function getReorderHandle(header: ElementHarness) { + return header.find(`.${Classes.TABLE_REORDER_HANDLE_TARGET}`); } }); From 186cce93da746f620ff86f09d31851e5b00b5ce4 Mon Sep 17 00:00:00 2001 From: Chris Lewis Date: Thu, 17 Aug 2017 12:02:05 -0700 Subject: [PATCH 10/18] Try to fix stylelint errors --- .../table/src/interactions/_interactions.scss | 25 ++++++++----------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/packages/table/src/interactions/_interactions.scss b/packages/table/src/interactions/_interactions.scss index bd8484f5aa..a83c6198d3 100644 --- a/packages/table/src/interactions/_interactions.scss +++ b/packages/table/src/interactions/_interactions.scss @@ -50,29 +50,26 @@ $reorder-handle-width: 15px !default; } } -// break up this selector to reduce line length -.bp-table-column-header-cell { - &.bp-table-column-header-cell-has-reorder-handle:not(.bp-table-column-header-cell-has-interaction-bar) { - .bp-table-column-name-text { - padding-left: $reorder-handle-width; - } +// we either need a long selector or a deeply nested selector here. prefer the former. +// stylelint-disable max-line-length +.bp-table-column-header-cell.bp-table-column-header-cell-has-reorder-handle:not(.bp-table-column-header-cell-has-interaction-bar) { + .bp-table-column-name-text { + padding-left: $reorder-handle-width; + } - // keep the editable cell input flush with the cell's left border - .bp-table-column-name .bp-table-editable-name.pt-editable-text { - &::before { - left: -$reorder-handle-width; - } - } + // keep the editable cell input flush with the cell's left border + .bp-table-column-name .bp-table-editable-name.pt-editable-text::before { + left: -$reorder-handle-width; } } .bp-table-reorder-handle-target { @include grabbable(); + display: flex; position: absolute; top: 0; - left: 0; bottom: 0; - display: flex; + left: 0; align-items: center; justify-content: center; width: $reorder-handle-width; From 8cc9c2df8ad4880107edb7a668d374a8710185e0 Mon Sep 17 00:00:00 2001 From: Chris Lewis Date: Thu, 17 Aug 2017 12:51:35 -0700 Subject: [PATCH 11/18] Try again --- packages/table/src/interactions/_interactions.scss | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/table/src/interactions/_interactions.scss b/packages/table/src/interactions/_interactions.scss index a83c6198d3..6c3dc37951 100644 --- a/packages/table/src/interactions/_interactions.scss +++ b/packages/table/src/interactions/_interactions.scss @@ -66,12 +66,12 @@ $reorder-handle-width: 15px !default; .bp-table-reorder-handle-target { @include grabbable(); display: flex; + align-items: center; + justify-content: center; position: absolute; top: 0; bottom: 0; left: 0; - align-items: center; - justify-content: center; width: $reorder-handle-width; color: $gray3; From e2c34f72780ac0cf9a0263af20d797e49288d8e3 Mon Sep 17 00:00:00 2001 From: Chris Lewis Date: Mon, 21 Aug 2017 12:23:10 -0700 Subject: [PATCH 12/18] Shorter CSS classes --- packages/table/src/common/classes.ts | 4 ++-- packages/table/src/headers/columnHeaderCell.tsx | 4 ++-- packages/table/src/interactions/_interactions.scss | 4 +--- 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/packages/table/src/common/classes.ts b/packages/table/src/common/classes.ts index 9ce62da655..bd3ec86b01 100644 --- a/packages/table/src/common/classes.ts +++ b/packages/table/src/common/classes.ts @@ -18,14 +18,14 @@ export const TABLE_CELL_LEDGER_ODD = "bp-table-cell-ledger-odd"; export const TABLE_COLUMN_HEADER_TR = "bp-table-column-header-tr"; export const TABLE_COLUMN_HEADERS = "bp-table-column-headers"; export const TABLE_COLUMN_HEADER_CELL = "bp-table-column-header-cell"; -export const TABLE_COLUMN_HEADER_CELL_HAS_REORDER_HANDLE = "bp-table-column-header-cell-has-reorder-handle"; -export const TABLE_COLUMN_HEADER_CELL_HAS_INTERACTION_BAR = "bp-table-column-header-cell-has-interaction-bar"; export const TABLE_COLUMN_NAME = "bp-table-column-name"; export const TABLE_COLUMN_NAME_TEXT = "bp-table-column-name-text"; export const TABLE_CONTAINER = "bp-table-container"; export const TABLE_DRAGGING = "bp-table-dragging"; export const TABLE_EDITABLE_NAME = "bp-table-editable-name"; export const TABLE_FOCUS_REGION = "bp-table-focus-region"; +export const TABLE_HAS_INTERACTION_BAR = "bp-table-has-interaction-bar"; +export const TABLE_HAS_REORDER_HANDLE = "bp-table-has-reorder-handle"; export const TABLE_HEADER = "bp-table-header"; export const TABLE_HEADER_ACTIVE = "bp-table-header-active"; export const TABLE_HEADER_CONTENT = "bp-table-header-content"; diff --git a/packages/table/src/headers/columnHeaderCell.tsx b/packages/table/src/headers/columnHeaderCell.tsx index b747a224c7..4c90e231fe 100644 --- a/packages/table/src/headers/columnHeaderCell.tsx +++ b/packages/table/src/headers/columnHeaderCell.tsx @@ -124,8 +124,8 @@ export class ColumnHeaderCell extends AbstractComponent Date: Mon, 21 Aug 2017 12:52:12 -0700 Subject: [PATCH 13/18] [Experimental] On handle mousedown, select only the region that will be reordered --- packages/table/src/interactions/reorderable.tsx | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/table/src/interactions/reorderable.tsx b/packages/table/src/interactions/reorderable.tsx index e26224a422..8c7fd9740f 100644 --- a/packages/table/src/interactions/reorderable.tsx +++ b/packages/table/src/interactions/reorderable.tsx @@ -122,20 +122,21 @@ export class DragReorderable extends React.Component { // cache for easy access later in the lifecycle const selectedInterval = isRowHeader ? selectedRegion.rows : selectedRegion.cols; + this.selectedRegionStartIndex = selectedInterval[0]; // add 1 because the selected interval is inclusive, which simple subtraction doesn't // account for (e.g. in a FULL_COLUMNS range from 3 to 6, 6 - 3 = 3, but the selection // actually includes four columns: 3, 4, 5, and 6) this.selectedRegionLength = selectedInterval[1] - selectedInterval[0] + 1; } else { - // select the new region to avoid complex and unintuitive UX w/r/t the existing selection - this.props.onSelection([region]); - const regionRange = isRowHeader ? region.rows : region.cols; this.selectedRegionStartIndex = regionRange[0]; this.selectedRegionLength = regionRange[1] - regionRange[0] + 1; } + // the region to be reordered becomes the only selection + this.props.onSelection([region]); + return true; } From 8540bdc0da03f302323f9c29ed71b4904d743b39 Mon Sep 17 00:00:00 2001 From: Chris Lewis Date: Mon, 21 Aug 2017 12:55:16 -0700 Subject: [PATCH 14/18] Revert "[Experimental] On handle mousedown, select only the region that will be reordered" This reverts commit bdcc8458ed81be529b6373b08e2111102828b566. Actually, this leads to some buggy behavior with overlapping regions. Will consider this again later. --- packages/table/src/interactions/reorderable.tsx | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/table/src/interactions/reorderable.tsx b/packages/table/src/interactions/reorderable.tsx index 8c7fd9740f..e26224a422 100644 --- a/packages/table/src/interactions/reorderable.tsx +++ b/packages/table/src/interactions/reorderable.tsx @@ -122,21 +122,20 @@ export class DragReorderable extends React.Component { // cache for easy access later in the lifecycle const selectedInterval = isRowHeader ? selectedRegion.rows : selectedRegion.cols; - this.selectedRegionStartIndex = selectedInterval[0]; // add 1 because the selected interval is inclusive, which simple subtraction doesn't // account for (e.g. in a FULL_COLUMNS range from 3 to 6, 6 - 3 = 3, but the selection // actually includes four columns: 3, 4, 5, and 6) this.selectedRegionLength = selectedInterval[1] - selectedInterval[0] + 1; } else { + // select the new region to avoid complex and unintuitive UX w/r/t the existing selection + this.props.onSelection([region]); + const regionRange = isRowHeader ? region.rows : region.cols; this.selectedRegionStartIndex = regionRange[0]; this.selectedRegionLength = regionRange[1] - regionRange[0] + 1; } - // the region to be reordered becomes the only selection - this.props.onSelection([region]); - return true; } From f7509ac2481c15cd11c91fafc5cc16ef57d7ee54 Mon Sep 17 00:00:00 2001 From: Antoine Llorca Date: Mon, 21 Aug 2017 16:38:20 -0700 Subject: [PATCH 15/18] Square off icon margin --- packages/table/src/interactions/_interactions.scss | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/table/src/interactions/_interactions.scss b/packages/table/src/interactions/_interactions.scss index 6c3dc37951..4edf0b9e13 100644 --- a/packages/table/src/interactions/_interactions.scss +++ b/packages/table/src/interactions/_interactions.scss @@ -9,7 +9,9 @@ $resize-handle-padding: 2px !default; $resize-handle-color: $pt-intent-primary !default; $resize-handle-dragging-color: $pt-intent-primary !default; -$reorder-handle-width: 15px !default; +// we want to square off the margin around the handle +// so we need a special value based on the handle icon shape +$reorder-handle-width: 22px !default; @mixin grabbable() { cursor: grab; @@ -73,14 +75,14 @@ $reorder-handle-width: 15px !default; bottom: 0; left: 0; width: $reorder-handle-width; - color: $gray3; + color: $pt-text-color-disabled; &:hover { - color: $pt-text-color-muted; + color: $pt-icon-color-hover; } &:active { - color: $pt-text-color; + color: $pt-intent-primary; } } From 6c39f1f5d902af4f5f17cf1274b3f330e7046fc8 Mon Sep 17 00:00:00 2001 From: Chris Lewis Date: Tue, 22 Aug 2017 12:25:52 -0700 Subject: [PATCH 16/18] Respond to CR feedback --- packages/table/src/interactions/_interactions.scss | 2 +- packages/table/test/columnHeaderCellTests.tsx | 2 +- packages/table/test/tableTests.tsx | 9 ++++----- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/packages/table/src/interactions/_interactions.scss b/packages/table/src/interactions/_interactions.scss index 382feea324..c58c6e2ea4 100644 --- a/packages/table/src/interactions/_interactions.scss +++ b/packages/table/src/interactions/_interactions.scss @@ -56,7 +56,7 @@ $reorder-handle-width: 15px !default; } // keep the editable cell input flush with the cell's left border - .bp-table-column-name .bp-table-editable-name.pt-editable-text::before { + .bp-table-editable-name::before { left: -$reorder-handle-width; } } diff --git a/packages/table/test/columnHeaderCellTests.tsx b/packages/table/test/columnHeaderCellTests.tsx index 458f53d960..5983fc45a9 100644 --- a/packages/table/test/columnHeaderCellTests.tsx +++ b/packages/table/test/columnHeaderCellTests.tsx @@ -143,7 +143,7 @@ describe("", () => { }); describe("Reorder handle", () => { - const REORDER_HANDLE_CLASS = "bp-table-reorder-handle-target"; + const REORDER_HANDLE_CLASS = Classes.TABLE_REORDER_HANDLE_TARGET; it("shows reorder handle in interaction bar if reordering and interaction bar are enabled", () => { const element = mount({ useInteractionBar: true, isColumnReorderable: true }); diff --git a/packages/table/test/tableTests.tsx b/packages/table/test/tableTests.tsx index a550605f0d..461d0f637e 100644 --- a/packages/table/test/tableTests.tsx +++ b/packages/table/test/tableTests.tsx @@ -699,9 +699,9 @@ describe("
", () => { const OFFSET_X = (NEW_INDEX + LENGTH) * WIDTH_IN_PX - REORDER_HANDLE_WIDTH_IN_PX; const OFFSET_Y = (NEW_INDEX + LENGTH) * HEIGHT_IN_PX; - const onColumnsReordered: Sinon.SinonSpy = sinon.spy(); - const onRowsReordered: Sinon.SinonSpy = sinon.spy(); - const onSelection: Sinon.SinonSpy = sinon.spy(); + const onColumnsReordered = sinon.spy(); + const onRowsReordered = sinon.spy(); + const onSelection = sinon.spy(); afterEach(() => { onColumnsReordered.reset(); @@ -765,8 +765,7 @@ describe("
", () => { isColumnReorderable: true, onColumnsReordered, }); - const headerCell = getHeaderCell(getColumnHeadersWrapper(table), 0); - headerCell + getHeaderCell(getColumnHeadersWrapper(table), 0) .mouse("mousedown") .mouse("mousemove", OFFSET_X) .mouse("mouseup", OFFSET_X); From c75479042ac903797cc11ab3a6a3a7568b7380d6 Mon Sep 17 00:00:00 2001 From: Chris Lewis Date: Tue, 22 Aug 2017 13:15:23 -0700 Subject: [PATCH 17/18] Cleaner test setup now that handle is wider --- packages/table/test/tableTests.tsx | 66 ++++++++++++++++-------------- 1 file changed, 36 insertions(+), 30 deletions(-) diff --git a/packages/table/test/tableTests.tsx b/packages/table/test/tableTests.tsx index 461d0f637e..9e5afba0ae 100644 --- a/packages/table/test/tableTests.tsx +++ b/packages/table/test/tableTests.tsx @@ -680,24 +680,22 @@ describe("
", () => { }); describe("Reordering", () => { - // Phantom renders the table at a fixed width regardless of the number of columns. if - // NEW_INDEX is too big, we risk simulating mouse events that fall outside of the table - // bounds, which causes tests to fail. const OLD_INDEX = 0; const NEW_INDEX = 1; const LENGTH = 2; const NUM_COLUMNS = 5; const NUM_ROWS = 5; - const REORDER_HANDLE_WIDTH_IN_PX = 15; + // considerations: + // - make the rows and columns fit exactly in the table dimensions; not trying to test scrolling. + // - ensure the columns are wide enough to fit their reorder handle. + const CONTAINER_WIDTH_IN_PX = 200; + const CONTAINER_HEIGHT_IN_PX = 200; + const ROW_HEIGHT_IN_PX = CONTAINER_HEIGHT_IN_PX / NUM_ROWS; + const COLUMN_WIDTH_IN_PX = CONTAINER_WIDTH_IN_PX / NUM_COLUMNS; - // table hardcodes itself to 60px tall in Phantom, so use small enough sizes to ensure - // all rows fit. also, ensure the columns are wide enough to fit their reorder handle. - const HEIGHT_IN_PX = 5; - const WIDTH_IN_PX = 20; - - const OFFSET_X = (NEW_INDEX + LENGTH) * WIDTH_IN_PX - REORDER_HANDLE_WIDTH_IN_PX; - const OFFSET_Y = (NEW_INDEX + LENGTH) * HEIGHT_IN_PX; + const OFFSET_X = (NEW_INDEX + LENGTH) * COLUMN_WIDTH_IN_PX; + const OFFSET_Y = (NEW_INDEX + LENGTH) * ROW_HEIGHT_IN_PX; const onColumnsReordered = sinon.spy(); const onRowsReordered = sinon.spy(); @@ -717,12 +715,12 @@ describe("
", () => { }); const headerCell = getHeaderCell(getColumnHeadersWrapper(table), 0); const reorderHandle = getReorderHandle(headerCell); - reorderHandle.mouse("mousedown").mouse("mousemove", OFFSET_X); + reorderHandle.mouse("mousedown").mouse("mousemove", getAdjustedOffsetX(OFFSET_X, reorderHandle)); const guide = table.find(`.${Classes.TABLE_VERTICAL_GUIDE}`); expect(guide).to.exist; - reorderHandle.mouse("mouseup", OFFSET_X); + reorderHandle.mouse("mouseup", getAdjustedOffsetX(OFFSET_X, reorderHandle)); expect(onColumnsReordered.called).to.be.true; expect(onColumnsReordered.calledWith(OLD_INDEX, NEW_INDEX, LENGTH)).to.be.true; }); @@ -754,8 +752,8 @@ describe("
", () => { const reorderHandle = getReorderHandle(headerCell); reorderHandle .mouse("mousedown") - .mouse("mousemove", OFFSET_X) - .mouse("mouseup", OFFSET_X); + .mouse("mousemove", getAdjustedOffsetX(OFFSET_X, reorderHandle)) + .mouse("mouseup", getAdjustedOffsetX(OFFSET_X, reorderHandle)); expect(onColumnsReordered.called).to.be.true; expect(onSelection.firstCall.calledWith([Regions.column(0)])); }); @@ -785,10 +783,11 @@ describe("
", () => { // now we can reorder the column one spot to the right const newIndex = 1; const length = 1; - const offsetX = (newIndex + length) * WIDTH_IN_PX - REORDER_HANDLE_WIDTH_IN_PX; + const offsetX = (newIndex + length) * COLUMN_WIDTH_IN_PX; + const adjustedOffsetX = getAdjustedOffsetX(offsetX, reorderHandle); reorderHandle.mouse("mousedown") - .mouse("mousemove", offsetX) - .mouse("mouseup", offsetX); + .mouse("mousemove", adjustedOffsetX) + .mouse("mouseup", adjustedOffsetX); // called once on mousedown (to select column 0), once on mouseup (to move the selection) expect(onSelection.calledTwice).to.be.true; @@ -798,18 +797,20 @@ describe("
", () => { function mountTable(props: Partial) { const table = harness.mount( -
- - - - - -
, +
+ + + + + + +
+
, ); return table; } @@ -829,6 +830,11 @@ describe("", () => { function getReorderHandle(header: ElementHarness) { return header.find(`.${Classes.TABLE_REORDER_HANDLE_TARGET}`); } + + function getAdjustedOffsetX(offsetX: number, reorderHandle: ElementHarness) { + // adjust the x coordinate to account for the rendered width of the reorder handle + return offsetX - reorderHandle.element.getBoundingClientRect().width; + } }); describe("Focused cell", () => { From 5c6725fb9953fcb623c23d83f777a49b9c344503 Mon Sep 17 00:00:00 2001 From: Chris Lewis Date: Wed, 23 Aug 2017 12:23:44 -0700 Subject: [PATCH 18/18] Fix reordering docs --- packages/table/src/docs.md | 60 ++++++++++++++++++++++++++++---------- 1 file changed, 45 insertions(+), 15 deletions(-) diff --git a/packages/table/src/docs.md b/packages/table/src/docs.md index 927a92ad69..70f08b369e 100644 --- a/packages/table/src/docs.md +++ b/packages/table/src/docs.md @@ -96,21 +96,51 @@ regular expression (`[a-zA-Z]`). If the content is invalid, a @### Reordering -The table supports column and row reordering via the `isColumnReorderable` and `isRowReorderable` -props, respectively. The table also requires the `FULL_COLUMNS` selection mode to be enabled for -column reordering and the `FULL_ROWS` selection mode to be enabled for row reordering; these can be -set via the `selectionModes` prop. - -To reorder a single row or column, first click its header cell to select it, then click and drag the -header cell again to move it elsewhere. Likewise, to reorder multiple consecutive rows or columns at -once, click and drag across multiple header cells to select the range, then click and drag anywhere in -the selected header cells to move them as a group. - -
-
Column reordering with interaction bar enabled
- When the interaction bar is enabled, the table will show handle icons in the interaction bar that - you can drag directly without having to make a selection first. -
+The table supports drag-reordering of columns and rows via the `isColumnReorderable` and `isRowReorderable` +props, respectively. + +#### Reordering columns + +When `isColumnReorderable={true}`, a drag handle will appear in the column header (or in the +interaction bar, if `useInteractionBar={true}`). + +##### Single column + +To reorder a single column, click and drag the desired column's drag handle to the left or right, +then release. This will work whether or not column selection is enabled. + +##### Multiple columns + +To allow reordering of multiple contiguous columns at once, first set the following additional +props: + +- `allowMultipleSelection={true}` +- `selectionModes={[RegionCardinality.FULL_COLUMNS, ...]}` + +Then drag-select the desired columns into a single selection, and grab any selected column's drag +handle to reorder the entire selected block. + +##### Edge cases + +With disjoint column selections (specified via Cmd or Ctrl + click), +only the selection containing the target drag handle will be reordered. All other +selections will be cleared afterward. + +Reordering a column contained in two overlapping selections will currently result in undefined +behavior. + +#### Reordering rows + +Rows do not have a drag handle, so they must be selected before reordering. To reorder a selection +of one or more rows, simply click and drag anywhere in a selected row header, then release. Note +that the following props must be set for row reordering to work: + +- `isRowHeaderShown={true}` +- `isRowReorderable={true}` +- `selectionModes={[RegionCardinality.FULL_ROWS, ...]}` +- `allowMultipleSelection={true}` (to optionally enable multi-row reordering) + +#### Example @reactExample TableReorderableExample