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

philcchen · 2017-05-10T15:22:51Z

Fixes #1070

Checklist

Include tests

Changes proposed in this pull request:

Add a method called resizeRowsByTallestCells that takes in an array of column indices to resize by a set of columns. If no array is provided, the method uses the columns in the viewport.

Screenshot

philcchen · 2017-05-10T15:35:21Z

packages/table/test/tableTests.tsx

-    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;


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

gscshoyru · 2017-05-10T17:27:39Z

packages/table/src/table.tsx

+            const tallestByColumns = columnIndices.map((col) => locator.getTallestVisibleCellInColumn(col));
+            tallest = Math.max(...tallestByColumns);
+        } else {
+            // Use viewport columns


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

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

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

giladgray · 2017-05-10T17:39:41Z

packages/table/src/table.tsx

+     * Resize rows based on a given set of column indices.
+     * If no indices are provided, default to using the columns that are currently in the viewport.
+     */
+    public resizeRowsByTallestCells(columnIndices?: number[]) {


how about one method to rule them all:

public resizeRowsByTallestCells(...columnIndices: number[]) { } resizeRowsByTallestCells(1); resizeRowsByTallestCells(1, 2, 3);

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

ok that's fair i suppose

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

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.

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.

I like that suggestion @cmslewis with respect to the API

cmslewis

Left some thoughts about API design and comment language. Excited to get this in!

cmslewis · 2017-05-10T17:51:54Z

packages/table/src/table.tsx

+     * Resize rows based on a given set of column indices.
+     * If no indices are provided, default to using the columns that are currently in the viewport.
+     */
+    public resizeRowsByTallestCells(columnIndices?: number[]) {


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

cmslewis · 2017-05-10T18:03:51Z

packages/table/src/table.tsx

@@ -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.


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. */

cmslewis · 2017-05-10T18:12:10Z

packages/table/src/table.tsx

+     * Resize rows based on a given set of column indices.
+     * If no indices are provided, default to using the columns that are currently in the viewport.
+     */
+    public resizeRowsByTallestCells(columnIndices?: number[]) {


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.

cmslewis · 2017-05-10T18:13:33Z

packages/table/src/table.tsx

+            const tallestByColumns = columnIndices.map((col) => locator.getTallestVisibleCellInColumn(col));
+            tallest = Math.max(...tallestByColumns);
+        } else {
+            // Use viewport columns


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

cmslewis · 2017-05-10T18:14:14Z

packages/table/src/table.tsx

        const rowHeights = Array(this.state.rowHeights.length).fill(tallest);
        this.invalidateGrid();
        this.setState({ rowHeights });
    }

+    public resizeRowsByTallestCell(columnIndex: number) {


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

cmslewis

Requesting changes to make sure we discuss before merging.

philcchen · 2017-05-10T19:08:24Z

Unify api calls for resizing and improve comments for accuracy

Preview: documentation | landing | table
Coverage: core | datetime

philcchen · 2017-05-10T19:10:21Z

Re-add removed line

Preview: documentation | landing | table
Coverage: core | datetime

cmslewis · 2017-05-10T19:21:37Z

packages/table/src/table.tsx

-    public resizeRowsByTallestCell(columnIndex: number) {
+    /**
+     * 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.


My mistake: do we need to clarify "tallest visible cell" in this sentence too, or is it implicitly clear?

I think the "in view" satisfies that constraint, but to y'all. Happy to make the change if it's more clear?

Can we change to default to using the tallest visible cell from all columns in view? That way it doesn't really get any longer.

philcchen · 2017-05-10T20:38:29Z

Update comment for clarity

Preview: documentation | landing | table
Coverage: core | datetime

giladgray · 2017-05-10T20:59:09Z

packages/table/test/tableTests.tsx

        expect(table.state.rowHeights[0]).to.equal(MAX_HEIGHT);

        // Resize by second column via array
-        table.resizeRowsByTallestCells([1]);
+        table.resizeRowsByTallestCell([1]);


can you remove brackets here to test non-array syntax? (1)

oh nvm i see the lines above now 👍

adidahiya · 2017-05-10T21:18:38Z

none of the tables load in the table preview for me

philcchen · 2017-05-10T21:20:21Z

@adidahiya yeah, not sure, maybe cause I'm on a fork?

adidahiya · 2017-05-10T21:32:58Z

:( sad times. I dunno if that's the cause but I gave you write access anyway so you can try it out in this repo.

cmslewis

Thanks for making those changes. 👍

philcchen · 2017-05-11T00:27:00Z

Closed in favor of #1078

Add resizeRowsByTallestCells to handle multi-column resize

ea06eb8

philcchen changed the title ~~Add resizeRowsByTallestCells to handle multi-column resize~~ [Table] Add resizeRowsByTallestCells to handle multi-column resize May 10, 2017

philcchen commented May 10, 2017

View reviewed changes

gscshoyru reviewed May 10, 2017

View reviewed changes

gscshoyru approved these changes May 10, 2017

View reviewed changes

giladgray reviewed May 10, 2017

View reviewed changes

cmslewis reviewed May 10, 2017

View reviewed changes

cmslewis suggested changes May 10, 2017

View reviewed changes

Phil Chen added 2 commits May 10, 2017 15:00

Unify api calls for resizing and improve comments for accuracy

9da8944

Re-add removed line

4f0680a

cmslewis suggested changes May 10, 2017

View reviewed changes

Update comment for clarity

77e0bca

giladgray reviewed May 10, 2017

View reviewed changes

cmslewis approved these changes May 10, 2017

View reviewed changes

philcchen closed this May 11, 2017

philcchen deleted the pchen/multi-column-resize branch May 11, 2017 00:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

philcchen commented May 10, 2017

philcchen May 10, 2017

gscshoyru May 10, 2017

philcchen May 10, 2017

cmslewis May 10, 2017

giladgray May 10, 2017 •

edited

Loading

adidahiya May 10, 2017

giladgray May 10, 2017

cmslewis May 10, 2017

cmslewis May 10, 2017

giladgray May 10, 2017

philcchen May 10, 2017

cmslewis left a comment

cmslewis May 10, 2017

cmslewis May 10, 2017

cmslewis May 10, 2017

cmslewis May 10, 2017

cmslewis May 10, 2017

cmslewis left a comment

philcchen commented May 10, 2017

philcchen commented May 10, 2017

cmslewis May 10, 2017

philcchen May 10, 2017

cmslewis May 10, 2017

philcchen commented May 10, 2017

giladgray May 10, 2017

giladgray May 10, 2017

adidahiya commented May 10, 2017

philcchen commented May 10, 2017

adidahiya commented May 10, 2017

cmslewis left a comment

philcchen commented May 11, 2017

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

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

Conversation

philcchen commented May 10, 2017

Fixes #1070

Checklist

Changes proposed in this pull request:

Screenshot

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giladgray May 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmslewis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmslewis left a comment

Choose a reason for hiding this comment

philcchen commented May 10, 2017

Unify api calls for resizing and improve comments for accuracy

philcchen commented May 10, 2017

Re-add removed line

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philcchen commented May 10, 2017

Update comment for clarity

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adidahiya commented May 10, 2017

philcchen commented May 10, 2017

adidahiya commented May 10, 2017

cmslewis left a comment

Choose a reason for hiding this comment

philcchen commented May 11, 2017

giladgray May 10, 2017 •

edited

Loading