Add table and column loading options #473

michael-yx-wu · 2017-01-12T23:41:11Z

Checklist

Include tests
Update documentation

Changes proposed in this pull request:

Make it easier to set the loading state on common sections of the table such as the headers and body by specifying loading options on Table and Column, components higher up in the hierarchy. Only exposing a loading prop on cell components results in needing to define a cellRenderer function just to conditionally set the loading state.

When loadingOptions are specified on both Table and Column and loading is also specified on Cell, the lowest component takes precedence. This allows users to manually make changes to the overall loading appearance of the table.

In the following example,

private render() {
  <Table loadingOptions={[ "cells", "column-header" ]}>
    <Column loadingOptions={[ "cells" ]} />
    <Column />
    <Column />
  </Table>
}

private cellRenderer = (row: number, col: number) => {
  return <Cell loading={row === 0 && col === 0} />;
}

All column / row headers and body cells show the loadings state except:

col1 header cell
all col1 cells except the first one

Use PureRender on Resizable and avoid passing a new function as props on each render in Table.

This change will require some clever changes to SelectionTests

The blasphemy!

blueprint-bot · 2017-01-12T23:45:43Z

Implement loading behavior in headers and body

Preview: docs | table
Coverage: core | datetime

blueprint-bot · 2017-01-13T01:02:00Z

Set default column loading options

Preview: docs | table
Coverage: core | datetime

blueprint-bot · 2017-01-13T01:37:18Z

Merge branch 'mw/table-header-loading' into mw/table-loading-options

Preview: docs | table
Coverage: core | datetime

michael-yx-wu · 2017-01-27T07:22:00Z

packages/table/test/loadingOptionsTests.tsx

+    return listOfSubsets;
+}
+
+function testLoadingOptionOverrides(


this test describes the expected loading option override behavior for all types of cells. is it clear that this is the case, or should i leave a comment?

comments are always appreciated, esp in a file with so much complex code.

i suspect there are some nice refactors that could be done to clean this up, but i recognize that you're basically out of time here :(

i refactored a bit before i put this commit up. i think this is probably easiest to read version of this function.

The expect cell loading function in `cellTestUtils.ts` now makes a more complete set of assertions given the expected loading state of a cell. This commit also introduces an exhaustive set tests for all possible loading option combinations.

blueprint-bot · 2017-01-27T07:28:31Z

Add exhaustive list of loading option tests

Preview: docs | table
Coverage: core | datetime

giladgray · 2017-01-27T19:30:08Z

packages/table/test/loadingOptionsTests.tsx

+    return listOfSubsets;
+}
+
+function testLoadingOptionOverrides(


comments are always appreciated, esp in a file with so much complex code.

i suspect there are some nice refactors that could be done to clean this up, but i recognize that you're basically out of time here :(

giladgray · 2017-01-27T19:31:37Z

packages/table/src/table.tsx

+            loading = cellLoading;
+        } else {
+            loading = this.hasLoadingOption(columnProps.loadingOptions, ColumnLoadingOption.CELLS);
+        }


i tend to prefer ternary ifs for this pattern so you can define the variable as a const and immediately assign the correct value.

const loading = (cellLoading == null) ? this.hasLoadingOption(columnProps.loadingOptions, ColumnLoadingOption.CELLS) : cellLoading;

giladgray · 2017-01-27T19:33:41Z

packages/table/test/loadingOptionsTests.tsx

+     */
+    allTableLoadingOptions.forEach((tableLoadingOptions: TableLoadingOption[]) => {
+        allColumnLoadingOptions.forEach((columnLoadingOptions: ColumnLoadingOption[]) => {
+            allRowLoadingOptions.forEach((rowLoadingOptions: RowLoadingOption[]) => {


shouldn't need types on the params here. can they not be inferred from forEach?

giladgray · 2017-01-27T19:33:58Z

packages/table/test/loadingOptionsTests.tsx

+        allColumnLoadingOptions.forEach((columnLoadingOptions: ColumnLoadingOption[]) => {
+            allRowLoadingOptions.forEach((rowLoadingOptions: RowLoadingOption[]) => {
+                it(`table: ${tableLoadingOptions}, column: ${columnLoadingOptions}, row: ${rowLoadingOptions}`, () => {
+                    const isCellLoading = (index: number) => {


pull this function out (define at root level) cuz it's essentially static.

giladgray · 2017-01-27T19:35:04Z

packages/table/test/loadingOptionsTests.tsx

+                                renderColumnHeader={renderColumnHeader}
+                            />
+                        </Table>,
+                    );


could even define a quick helper component <TableLoadingOptionsTester> that does this composition, so it doesn't have to be redefined in each test run.

giladgray · 2017-01-27T19:35:53Z

packages/table/test/loadingOptionsTests.tsx

+    cellType: CellType,
+    cellLoading: (index: number) => boolean,
+    columnLoadingOptions: ColumnLoadingOption[],
+    tableLoadingOptions: TableLoadingOption[]) {


put closing paren in line with open:

function name( args, ..., ) {

giladgray · 2017-01-27T19:37:07Z

packages/table/test/loadingOptionsTests.tsx

+    columnLoadingOptions: ColumnLoadingOption[],
+    tableLoadingOptions: TableLoadingOption[]) {
+
+    // tslint:disable-next-line:prefer-for-of


shouldn't need to disable this? seems like valid usage.

giladgray · 2017-01-27T19:38:56Z

packages/table/test/tableTests.tsx

+            expectCellLoading(columnHeaders.item(i), CellType.COLUMN_HEADER);
+        }
+
+        const rowHeaders = tableHarness.element.querySelectorAll(".bp-table-row-headers .bp-table-header");


you could use .queryAll() instead of querySelectorAll() (from dom4 polyfill) which returns an Array, then simply do rowHeaders.forEach(...)

giladgray · 2017-01-27T19:41:39Z

packages/table/test/cellTestUtils.ts

+            expect(headerNameText.children.length).to.equal(1);
+        } else {
+            expect(cell.children.length).to.equal(1);
+            expect(cell.querySelector(`.${Classes.SKELETON}`)).to.not.be.null;


this is identical to line 24

giladgray · 2017-01-27T19:41:53Z

packages/table/test/cellTestUtils.ts

+                : cell.querySelector(".bp-table-row-name");
+            expect(headerNameText).to.not.be.null;
+            expect(headerNameText.textContent).to.equal("");
+            expect(headerNameText.children.length).to.equal(1);


what is the one child? .pt-skeleton? can you make that explicit?

added a test that the child is a skeleton

blueprint-bot · 2017-01-27T20:41:21Z

Be explicit about single child loading condition

Preview: docs | table
Coverage: core | datetime

blueprint-bot · 2017-01-27T20:55:20Z

Include undefined loading options

Preview: docs | table
Coverage: core | datetime

giladgray · 2017-01-27T21:03:42Z

packages/table/test/cellTestUtils.ts

        } else {
            expect(cell.children.length).to.equal(1);
-            expect(cell.querySelector(`.${Classes.SKELETON}`)).to.not.be.null;
+            expect(cell.children[0].classList.contains(Classes.SKELETON)).to.not.be.null;


wait why back to this format?
did my comments lead us in a circle? 😶

you're testing too much here and it's not helping.
it should be sufficient to assert that a selector exists, for example:
.bp-table-row-name .pt-skeleton

i wanted to be explicit about where the skeleton exists, hence the single child test. it's nested further down in header cells vs. normal cells

L31 and L34 are slightly different in this sense.

blueprint-bot · 2017-01-27T21:32:46Z

Include undefined loading options

Preview: docs | table
Coverage: core | datetime

giladgray · 2017-01-27T21:40:52Z

😅 let's do this!

blueprint-bot · 2017-01-27T21:42:15Z

Stop relying on dom structure in cell test

Preview: docs | table
Coverage: core | datetime

michael-yx-wu added 21 commits January 10, 2017 16:32

Support header cell loading state

8092240

Update docs and example

1eed956

Fix css syntax bug

87aef15

Add column header cell styles

707072b

Fix row header styles

81a8a94

Render less

9600f2c

Use PureRender on Resizable and avoid passing a new function as props on each render in Table.

Fade in row header name

7179355

Clean up scss

1a817a9

Remove table rendering optimization

4659373

This change will require some clever changes to SelectionTests

Revert editable example

dbc7bba

Use mixin for hiding text

982ceb8

Make calculateStyles return styles

e212d9d

Render undefined instead of null

fb3117e

1-index rows

fd7457b

The blasphemy!

Use fat variable length column header skeletons

f7623d0

Shorten row header skeletons

0a3c7bf

Support passing in higher level loading props

7f9a28c

Clean up styles styles

dbe0801

Remove unused variables

1c99297

Merge branch 'master' into mw/table-header-loading

7ee1183

Set loading on headers and TableBody

777ec16

Implement loading behavior in headers and body

9b77cbe

michael-yx-wu force-pushed the mw/table-loading-options branch from 3ef3811 to 9b77cbe Compare January 13, 2017 00:05

michael-yx-wu added 2 commits January 12, 2017 16:55

Check that column loading options is not null

50c7ab6

Set default column loading options

a3e2b8e

Merge branch 'mw/table-header-loading' into mw/table-loading-options

a716afa

michael-yx-wu force-pushed the mw/table-loading-options branch from e668650 to f0b3e5e Compare January 16, 2017 20:32

michael-yx-wu added 3 commits January 26, 2017 15:44

Remove unnecessary if else

6004111

Remove arguments to emptyCellRenderer

b63575f

Actually test loading rendering behavior

75517ac

michael-yx-wu force-pushed the mw/table-loading-options branch from 32be46e to de72ce6 Compare January 27, 2017 07:17

michael-yx-wu commented Jan 27, 2017

View reviewed changes

Add exhaustive list of loading option tests

2c9ae40

The expect cell loading function in `cellTestUtils.ts` now makes a more complete set of assertions given the expected loading state of a cell. This commit also introduces an exhaustive set tests for all possible loading option combinations.

michael-yx-wu force-pushed the mw/table-loading-options branch from de72ce6 to 2c9ae40 Compare January 27, 2017 07:24

giladgray reviewed Jan 27, 2017

View reviewed changes

giladgray suggested changes Jan 27, 2017

View reviewed changes

giladgray added the P0 label Jan 27, 2017

michael-yx-wu added 2 commits January 27, 2017 14:48

Prefer ternary over if else assignment

3832858

Refactor loadingOptionsTests

7b97042

michael-yx-wu force-pushed the mw/table-loading-options branch from 7c363fa to 7b97042 Compare January 27, 2017 20:25

michael-yx-wu added 2 commits January 27, 2017 15:32

Use queryAll instead of querySelectorAll

0e97784

Be explicit about single child loading condition

01f83a0

giladgray reviewed Jan 27, 2017

View reviewed changes

Include undefined loading options

dd737f8

michael-yx-wu force-pushed the mw/table-loading-options branch from 04b97e6 to dd737f8 Compare January 27, 2017 21:27

Stop relying on dom structure in cell test

8af8cf4

giladgray approved these changes Jan 27, 2017

View reviewed changes

giladgray merged commit 5c7417b into master Jan 27, 2017

giladgray deleted the mw/table-loading-options branch January 27, 2017 21:55

giladgray mentioned this pull request Jan 27, 2017

Prepare Release 1.7.0 #563

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add table and column loading options #473

Add table and column loading options #473

michael-yx-wu commented Jan 12, 2017 •

edited

Loading

blueprint-bot commented Jan 12, 2017

blueprint-bot commented Jan 13, 2017

blueprint-bot commented Jan 13, 2017

michael-yx-wu Jan 27, 2017

giladgray Jan 27, 2017

michael-yx-wu Jan 27, 2017

blueprint-bot commented Jan 27, 2017

giladgray Jan 27, 2017

giladgray Jan 27, 2017

giladgray Jan 27, 2017

giladgray Jan 27, 2017

giladgray Jan 27, 2017

giladgray Jan 27, 2017

giladgray Jan 27, 2017

giladgray Jan 27, 2017

giladgray Jan 27, 2017

giladgray Jan 27, 2017

michael-yx-wu Jan 27, 2017

blueprint-bot commented Jan 27, 2017

blueprint-bot commented Jan 27, 2017

giladgray Jan 27, 2017

michael-yx-wu Jan 27, 2017 •

edited

Loading

michael-yx-wu Jan 27, 2017

blueprint-bot commented Jan 27, 2017

giladgray commented Jan 27, 2017

blueprint-bot commented Jan 27, 2017

Add table and column loading options #473

Add table and column loading options #473

Conversation

michael-yx-wu commented Jan 12, 2017 • edited Loading

Checklist

Changes proposed in this pull request:

blueprint-bot commented Jan 12, 2017

Implement loading behavior in headers and body

blueprint-bot commented Jan 13, 2017

Set default column loading options

blueprint-bot commented Jan 13, 2017

Merge branch 'mw/table-header-loading' into mw/table-loading-options

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueprint-bot commented Jan 27, 2017

Add exhaustive list of loading option tests

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

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

blueprint-bot commented Jan 27, 2017

Be explicit about single child loading condition

blueprint-bot commented Jan 27, 2017

Include undefined loading options

Choose a reason for hiding this comment

michael-yx-wu Jan 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blueprint-bot commented Jan 27, 2017

Include undefined loading options

giladgray commented Jan 27, 2017

blueprint-bot commented Jan 27, 2017

Stop relying on dom structure in cell test

michael-yx-wu commented Jan 12, 2017 •

edited

Loading

michael-yx-wu Jan 27, 2017 •

edited

Loading