[table] feat: add default implementation for getCellClipboardData

[bergkvist]

Fixes #4566

Checklist

• Includes tests
• Update documentation

Changes proposed in this pull request:

• Add default implementation of getCellClipboardData for Table and Table2, which uses the package react-innertext to extract the default value of each cell.

• Change signature of getCellClipboardData:

// Before:
getCellClipboardData?: (row: number, col: number) => any;

// After:
getCellClipboardData?: (row: number, col: number, tableState: TableState) => any;

[bergkvist]

It seems like there is only one test for getCellClipboardData, which is broken, so I haven't been able to write a test for this yet:

blueprint/packages/table/test/selectionTests.tsx

Lines 58 to 70 in 92db539

	// TODO: Fix
	it.skip("Copies selected cells when keys are pressed", () => {
	const onCopy = sinon.spy();
	const getCellClipboardData = Utils.toBase26CellName;
	const copyCellsStub = sinon.stub(Clipboard, "copyCells").returns(Promise.resolve());
	const table = harness.mount(createTableOfSize(3, 7, {}, { getCellClipboardData, onCopy }));
	table.find(COLUMN_TH_SELECTOR)!.mouse("mousedown").mouse("mouseup");
	table.find(COLUMN_TH_SELECTOR)!.focus();
	table.find(COLUMN_TH_SELECTOR)!.keyboard("keydown", "C", true);
	expect(copyCellsStub.lastCall.args).to.deep.equal([[["A1"], ["A2"], ["A3"], ["A4"], ["A5"], ["A6"], ["A7"]]]);
	expect(onCopy.lastCall.args).to.deep.equal([true]);
	});

[adidahiya]

I think this is a breaking change; we'll have to make the new prop argument optional if we want to add this to the API.

Also I think we should limit the scope of the new information required here. It would be better with just a cellRenderer argument rather than the whole table state.

Suggested change

	getCellClipboardData?: (row: number, col: number, tableState: TableState) => any;
	getCellClipboardData?: (row: number, col: number, cellRenderer?: CellRenderer) => any;

Lastly, I think we should add a note about the default implementation to the documentation for this prop, right above in the JSDoc.

[bergkvist]

tableState is not optional - since it is always defined/always passed by the caller. This happens here:

blueprint/packages/table/src/tableHotkeys.ts

Line 419 in 1f8ead7

const sparse = Regions.sparseMapCells(cells, (row, col) => getCellClipboardData(row, col, this.state));

It is not a breaking change to the API - since you don't need to use all the arguments when creating a callback function. Examples:

<>
  {/* Still possible after API change */}
  <Table getCellClipboardData={() => 'hello world'} ... />
  
  {/* Still possible after API change */}
  <Table getCellClipboardData={(row) => `row=${row}`} ... />

  {/* Still possible after API change */}
  <Table getCellClipboardData={(row, col) => `row=${row}, col=${col}`} ... />

  {/* Only possible after API change */}
  <Table getCellClipboardData={(row, col, tableState) => {
    return tableState.childrenArray[col].props.cellRenderer?.(row, col)
    // If tableState was optional - we would be forced to check if it is undefined in this callback:
    // return tableState?.childrenArray[col].props.cellRenderer?.(row, col)
  }} ... />
</>

Passing the entire tableState means the user of the Table-component will have a lot of flexibility when writing a custom getCellClipboardData-function. I see this as a bit analogous to how you have access to the entire array in the .map callback on arrays:

> [1,2,3,4].map((value, index, array) => array)
// Output:
[ [ 1, 2, 3, 4 ], [ 1, 2, 3, 4 ], [ 1, 2, 3, 4 ], [ 1, 2, 3, 4 ] ]

[bergkvist]

I updated the docstring now to mention the default implementation.

In terms of CellRenderer vs TableState, I guess this depends on whether TableState is likely to change in backwards-incompatible ways in the future or not. CellRenderer certainly exposes less surface area - at the cost of some flexibility (which might not be useful in practice). But is CellRenderer maybe too specific to the default implementation?

If we find out we need something else from TableState in the getCellClipboardData callback in the future, this would then have to be added as yet another argument to not break backwards compatibility - which could cause a very messy callback signature for getCellClipboardData over time.

[adidahiya]

Ok, I tested out the new argument and came to the same conclusion. It's not a breaking change like I imagined it would be with tsc --strict, so that's good 👍

As for API surface area... I generally try to be conservative about adding to the API surface of Blueprint, as it's already quite large. TableState is a type which I don't necessarily want to export as public API (you'll notice it's not even exported from the package root right now). It may have breaking changes in the future which I don't want to be constrained by.

So, yes, I would like to limit the scope of the data in the new argument here. Please change it to CellRenderer, and pass along the specific cell renderer for that row/col coordinate.

Suggested change

	getCellClipboardData?: (row: number, col: number, tableState: TableState) => any;
	getCellClipboardData?: (row: number, col: number, cellRenderer: CellRenderer) => any;

If we find out we need something else from TableState in the getCellClipboardData callback in the future, this would then have to be added as yet another argument to not break backwards compatibility - which could cause a very messy callback signature for getCellClipboardData over time.

I hear your concern here, and I also don't want to end up with an unwieldy callback signature over time. But at this point we only have 2 arguments, and I'm OK with adding a third one. I'll consider refactoring the next time we need to add more information here, if there is a next time.

[adidahiya]

Ok, I tested out the new argument and came to the same conclusion. It's not a breaking change like I imagined it would be with tsc --strict, so that's good 👍

As for API surface area... I generally try to be conservative about adding to the API surface of Blueprint, as it's already quite large. TableState is a type which I don't necessarily want to export as public API (you'll notice it's not even exported from the package root right now). It may have breaking changes in the future which I don't want to be constrained by.

So, yes, I would like to limit the scope of the data in the new argument here. Please change it to CellRenderer, and pass along the specific cell renderer for that row/col coordinate.

Suggested change

	getCellClipboardData?: (row: number, col: number, tableState: TableState) => any;
	getCellClipboardData?: (row: number, col: number, cellRenderer: CellRenderer) => any;

If we find out we need something else from TableState in the getCellClipboardData callback in the future, this would then have to be added as yet another argument to not break backwards compatibility - which could cause a very messy callback signature for getCellClipboardData over time.

I hear your concern here, and I also don't want to end up with an unwieldy callback signature over time. But at this point we only have 2 arguments, and I'm OK with adding a third one. I'll consider refactoring the next time we need to add more information here, if there is a next time.

[adidahiya]

@bergkvist I'd like to merge this in this week, so if you don't mind, I will apply my suggestions myself to this PR