Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Table] Add resizeRowsByTallestCells to handle multi-column resize #1075

Closed
wants to merge 4 commits into from
Closed

[Table] Add resizeRowsByTallestCells to handle multi-column resize #1075

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ea06eb8
Add resizeRowsByTallestCells to handle multi-column resize
May 10, 2017
9da8944
Unify api calls for resizing and improve comments for accuracy
May 10, 2017
4f0680a
Re-add removed line
May 10, 2017
77e0bca
Update comment for clarity
May 10, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/table/preview/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ <h4>Column Type Formats</h4>
<div class="resize-default pt-button pt-intent-success">Resize Default</div>
<div class="resize-wrapped pt-button pt-intent-success">Resize Wrapped Text</div>
<div class="resize-json-wrapped pt-button pt-intent-success">Resize Json Wrapped Text</div>
<div class="resize-wrapped-and-json pt-button pt-intent-success">Resize Wrapped and Json Text</div>
<div class="resize-viewport pt-button pt-intent-success">Resize Viewport</div>
<br /><br />

<h4>Editable Column Names and Cells</h4>
Expand Down
6 changes: 6 additions & 0 deletions packages/table/preview/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,12 @@ class FormatsTable extends React.Component<{}, {}> {
document.querySelector(".resize-json-wrapped").addEventListener("click", () => {
this.formatsTable.resizeRowsByTallestCell(3);
});
document.querySelector(".resize-wrapped-and-json").addEventListener("click", () => {
this.formatsTable.resizeRowsByTallestCells([1, 3]);
});
document.querySelector(".resize-viewport").addEventListener("click", () => {
this.formatsTable.resizeRowsByTallestCells();
});
}

private renderDefaultCell = (row: number) => <Cell>{this.strings[row]}</Cell>;
Expand Down
25 changes: 22 additions & 3 deletions packages/table/src/table.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,15 +489,34 @@ export class Table extends AbstractComponent<ITableProps, ITableState> {
);
}

public resizeRowsByTallestCell(columnIndex: number) {
/**
* Resize rows based on a given set of column indices.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I vote for a slightly more precise description here. Something like:

/**
 * Resize all rows in the table to the height of the tallest visible cell in the specified columns.
 * If no indices are provided, default to using the tallest cell from all columns currently in view.
 */

* If no indices are provided, default to using the columns that are currently in the viewport.
*/
public resizeRowsByTallestCells(columnIndices?: number[]) {
Copy link
Contributor

@giladgray giladgray May 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about one method to rule them all:

public resizeRowsByTallestCells(...columnIndices: number[]) { }

resizeRowsByTallestCells(1);
resizeRowsByTallestCells(1, 2, 3);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's a limit to the number of args you can provide to a function. better to leave this as an array. some people have tables that are 10,000s cols wide. if you really want a single function then you can overload it and do some runtime type checking on the argument but I think this is fine as-is

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok that's fair i suppose

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, never thought about that before. TIL http://stackoverflow.com/questions/22747068/is-there-a-max-number-of-arguments-javascript-functions-can-accept

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thought: I don't think this function is named quite correctly as is; no matter how many column indices you provide, you still only care about one "tallest cell" amongst all of them, right?

Is resizeRowsByTallestCell(columnIndices: number | number[]) out of the question? Then we could just follow a different code path if Arrays.isArray(columnIndices) === true.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i am inclined to agree with @cmslewis above. the plural felt weird to me to begin with, and now it's clear that it's not even semantically correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that suggestion @cmslewis with respect to the API

const { locator } = this.state;

const tallest = locator.getTallestVisibleCellInColumn(columnIndex);
let tallest = 0;
if (columnIndices != null) {
const tallestByColumns = columnIndices.map((col) => locator.getTallestVisibleCellInColumn(col));
tallest = Math.max(...tallestByColumns);
} else {
// Use viewport columns
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slightly worried that people will think it applies to everything if you leave this blank... but probably fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could create a separate, explicit method if that's preferable, since the usage might not be obvious; don't have strong opinions

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could use a slightly more detailed comment here. // Consider all columns currently in view

const { grid } = this;
const { viewportRect } = this.state;
const viewportColumnIndices = grid.getColumnIndicesInRect(viewportRect);
for (let col = viewportColumnIndices.columnIndexStart; col <= viewportColumnIndices.columnIndexEnd; col++) {
tallest = Math.max(tallest, locator.getTallestVisibleCellInColumn(col));
}
}
const rowHeights = Array(this.state.rowHeights.length).fill(tallest);
this.invalidateGrid();
this.setState({ rowHeights });
}

public resizeRowsByTallestCell(columnIndex: number) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Under my proposal above, we wouldn't need this additional function.

this.resizeRowsByTallestCells([columnIndex]);
}

/**
* When the component mounts, the HTML Element refs will be available, so
* we constructor the Locator, which queries the elements' bounding
Expand Down
30 changes: 25 additions & 5 deletions packages/table/test/tableTests.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,22 +103,42 @@ describe("<Table>", () => {
rowHeaders.forEach((rowHeader) => expectCellLoading(rowHeader, CellType.ROW_HEADER));
});

it("Gets and sets the tallest cell height in the table", () => {
const renderCell = () => <Cell wrapText={true}>my cell value with lots and lots of words</Cell>;
it("Gets and sets the tallest cell by columns correctly", () => {
const DEFAULT_RESIZE_HEIGHT = 30;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think DEFAULT_RESIZE_HEIGHT = 30 stems from the fact that the cell header height is 30; so it ends up resizing all of the cell row heights to being 30 (even though it doesn't need to). This is true even without this commit

const MAX_HEIGHT = 40;
const renderCellLong = () => <Cell wrapText={true}>my cell value with lots and lots of words</Cell>;
const renderCellShort = () => <Cell wrapText={false}>short value</Cell>;

let table: Table;

const saveTable = (t: Table) => table = t;

harness.mount(
<Table ref={saveTable} numRows={4}>
<Column name="Column0" renderCell={renderCell} />
<Column name="Column1" renderCell={renderCell} />
<Column name="Column0" renderCell={renderCellLong} />
<Column name="Column1" renderCell={renderCellShort} />
</Table>,
);

// Resize by first column
table.resizeRowsByTallestCell(0);
expect(table.state.rowHeights[0]).to.equal(40);
expect(table.state.rowHeights[0]).to.equal(MAX_HEIGHT);

// Resize by second column
table.resizeRowsByTallestCell(1);
expect(table.state.rowHeights[0]).to.equal(DEFAULT_RESIZE_HEIGHT);

// Resize by both columns
table.resizeRowsByTallestCells([0, 1]);
expect(table.state.rowHeights[0]).to.equal(MAX_HEIGHT);

// Resize by second column via array
table.resizeRowsByTallestCells([1]);
expect(table.state.rowHeights[0]).to.equal(DEFAULT_RESIZE_HEIGHT);

// Resize by visible columns
table.resizeRowsByTallestCells();
expect(table.state.rowHeights[0]).to.equal(MAX_HEIGHT);
});

it("Selects all on click of upper-left corner", () => {
Expand Down