Support loading prop on header cells #452

michael-yx-wu · 2017-01-11T02:28:06Z

Checklist

Works with EditableName
Works with children content (spacing only)
- Users will be responsible with using the pt-skeleton class on their header cell children
Update documentation
Includes tests

blueprint-bot · 2017-01-11T02:32:54Z

Update docs and example

Preview: docs | table
Coverage: core | datetime

blueprint-bot · 2017-01-11T02:39:54Z

Fix css syntax bug

Preview: docs | table
Coverage: core | datetime

giladgray

kinda want header skeletons to be a little taller, like the header text itself relative to cell text?

giladgray · 2017-01-11T19:23:53Z

packages/table/src/common/loadableContent.tsx

@@ -29,7 +29,7 @@ export class LoadableContent extends React.Component<ILoadableContentProps, {}>

    public constructor(props: ILoadableContentProps) {
        super(props);
-        this.calculateStyle(props.loading);
+        this.calculateStyle(props.variableLength);


preferable to have this function return the styles instead of setting them internally (#sideeffect).

this.style = this.calculateStyle()

giladgray · 2017-01-11T19:24:44Z

packages/table/src/headers/columnHeaderCell.tsx

@@ -59,6 +62,15 @@ export interface IColumnHeaderCellProps extends IColumnNameProps, IProps {
    isColumnSelected?: boolean;

    /**
+     * If true, the column name (string or `ReactElement`) will be replaced with a fixed-height


paren clause is not helpful, please remove. say "column name" to reference the prop, from which the user can infer the typings.

giladgray · 2017-01-11T19:26:39Z

packages/table/src/headers/columnHeaderCell.tsx

        return (
            <div className={classes} style={style}>
                {this.renderName()}
                {this.maybeRenderContent()}
-                {resizeHandle}
+                {loading ? null : resizeHandle}


use undefined instead of null in cases like this please

comments in this file also apply to file below

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

llorca · 2017-01-11T23:36:01Z

packages/table/examples/cellLoadingExample.tsx

-                for (let property of Object.getOwnPropertyNames(bigSpaceRocks[0])) {
-                    randomNumbers.push(Math.random());
-                }
+            for (let i = -1 ; i < numRows * numColumns; i++) {


Looks like an unnecessary space after -1

This change will require some clever changes to SelectionTests

The blasphemy!

blueprint-bot · 2017-01-12T02:11:17Z

Shorten row header skeletons

Preview: docs | table
Coverage: core | datetime

llorca · 2017-01-12T02:35:35Z

To fix the corner radius clipping caused by justify-contents on .bp-table-cell.pt-loading, we can try using an alternate way of centering the skeleton in the cell. Try removing justify-contents from .bp-table-cell.pt-loading, then add the following to .bp-table-cell .pt-skeleton:

position: absolute;
top: 50%;
transform: translateX(-50%);

I suppose it can also apply to other types of cells that need centering

giladgray

webpack errors on circle (these don't fail the build but should be fixed):

giladgray · 2017-01-12T20:25:23Z

packages/table/src/common/_loading.scss

+  pointer-events: none;
+}
+
+@mixin vertical-align() {


thing is, vertical-align is an actual CSS property which this mixin has nothing to do with.

vertical-center?

you're right that's confusing. renaming!

giladgray · 2017-01-12T20:26:37Z

packages/table/src/common/_loading.scss

  .pt-skeleton {
    opacity: 0;
-    height: $pt-grid-size / 2;
+    height: $skeleton-height * 1px;


whoa please just expect a unit from the variable. if the variable is called height then it should be usable as a height (ie, has a unit).

giladgray · 2017-01-12T20:29:56Z

packages/table/src/common/_loading.scss

+  }
+}
+
+@mixin cell-loading {


this mixin is used exactly once (in _cell.scss) but looks very similar to other instances of loading cells. suggests a potential refactor, though i'm not sure what...

blueprint-bot · 2017-01-12T22:39:56Z

Remove unused variables

Preview: docs | table
Coverage: core | datetime

blueprint-bot · 2017-01-24T20:54:24Z

Add column header loading test

Preview: docs | table
Coverage: core | datetime

giladgray

looking really good! a few questions below

giladgray · 2017-01-25T18:52:01Z

packages/table/src/common/_loading.scss

  .pt-skeleton {
    opacity: 0;
-    height: $pt-grid-size / 2;
+    height: $pt-grid-size * 0.5;


i much prefer / 2 to * 0.5. why this change?

giladgray · 2017-01-25T18:54:18Z

packages/table/src/headers/_headers.scss

+
+  .bp-table-column-name-text {
+    @include vertical-center();
+    padding: ($pt-grid-size * 1.1) $pt-grid-size ($pt-grid-size * 1.2);


whoa why the freaky padding?

i'd expect vertical-center to center correctly, to not require 1px adjustments.

it does center the children content of .bp-table-column-name-text. the default header is 30px high. the problem with a 7px height skeleton is that we need unequal top / bottom padding to make it 30px (i.e. 11px top, 22px bottom)

ok gotcha. in that case, would be even better to compute these values from the variable i proposed below. use floor() and ceil() functions to ensure whole pixel values.

also, what's wrong with 8px instead of 7? even numbers are easier.

the designers were against 8px, too thick. there was a bushy eyebrow meme going on for a while, because that's what it looks like 😛.

giladgray · 2017-01-25T18:54:49Z

packages/table/src/headers/_headers.scss

+    padding: ($pt-grid-size * 1.1) $pt-grid-size ($pt-grid-size * 1.2);
+
+    .pt-skeleton {
+      height: $pt-grid-size * 0.7;


this would make a good variable. $header-skeleton-height

giladgray · 2017-01-25T18:55:31Z

packages/table/src/headers/columnHeaderCell.tsx

+     * will also want to conditionally apply the `.pt-skeleton` class where appropriate.
+     * @default false
+     */
+    loading?: boolean;


huh awkward that it's inconsistent with the rest of the component 😢

llorca · 2017-01-25T19:01:49Z

packages/table/src/cell/_common.scss

@@ -29,7 +29,7 @@ $cell-transition-duration: $pt-transition-duration * 3;
  transition: color $cell-transition-duration;

  .pt-dark & {
-    &:not([class*="pt-intent-"]) {
+    &:not([class*="pt-intent-"]):not(.pt-loading) {


Isn't &:not([class*="pt-intent-"], .pt-loading) valid?

that syntax is proposed in selectors level 4: https://drafts.csswg.org/selectors-4/#negation

in selectors level 3, :not only takes a simple argument. so while this compiles and seems to work in chrome and FF, i'm on the latest stable version for both. i'm going to go with the more verbose syntax for now since it's guaranteed it'll work on older browsers

blueprint-bot · 2017-01-25T19:14:28Z

Clean up CSS

Preview: docs | table
Coverage: core | datetime

blueprint-bot · 2017-01-25T19:47:06Z

Use floor / ceil instead of custom padding

Preview: docs | table
Coverage: core | datetime

blueprint-bot · 2017-01-25T21:34:54Z

Rename vertical center mixin

Preview: docs | table
Coverage: core | datetime

blueprint-bot · 2017-01-25T22:19:17Z

Move vertical center mixin to cell/common

Preview: docs | table
Coverage: core | datetime

llorca · 2017-01-25T22:22:49Z

packages/table/src/cell/_common.scss

@@ -62,3 +62,9 @@ $cell-transition-duration: $pt-transition-duration * 3;
    cursor: $select-cell-cursor;
  }
 }
+
+@mixin cell-content-align-vertical() {


That clears up naming confusion, and describes what it does: vertically center anything inside a cell. Not specific to loading or header cells. How's that @giladgray?

blueprint-bot · 2017-01-25T22:23:57Z

Move vertical center mixin to cell/common

Preview: docs | table
Coverage: core | datetime

blueprint-bot · 2017-01-25T22:25:51Z

Rename mixin

Preview: docs | table
Coverage: core | datetime

giladgray · 2017-01-25T22:33:31Z

packages/table/src/headers/_headers.scss

+    @include cell-content-align-vertical();
+    padding: floor(($column-header-min-height - $column-name-text-skeleton-height) / 2)
+      $pt-grid-size
+      ceil(($column-header-min-height - $column-name-text-skeleton-height) / 2);


this entire property is actually worthless because of the mixin. try removing it and notice how the skeleton becomes perfectly centered (and how it was clearly a pixel off with this padding).

i'd still suggest 8px height cuz even numbers are always better for centering.

alternately, just set the super simple padding: $pt-grid-size; to ensure space on all sides, and let flex box handle the actual centering.

blueprint-bot · 2017-01-25T22:41:40Z

Change header height to 8px

Preview: docs | table
Coverage: core | datetime

giladgray · 2017-01-25T23:38:43Z

.vscode/settings.json

+    "typescript.tsdk": "./node_modules/typescript/lib",
+    "vsicons.presets.angular": false
+}


what happened to reverting this?

blueprint-bot · 2017-01-25T23:44:36Z

Change header height to 8px

Preview: docs | table
Coverage: core | datetime

michael-yx-wu · 2017-01-25T23:55:46Z

reverted the .vscode/settings change!

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

Support header cell loading state

8092240

Update docs and example

1eed956

Fix css syntax bug

87aef15

giladgray suggested changes Jan 11, 2017

View reviewed changes

llorca added Package: table Type: feature request labels Jan 11, 2017

michael-yx-wu added 3 commits January 11, 2017 14:16

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.

michael-yx-wu mentioned this pull request Jan 11, 2017

Broken transitions from table loading state to content #460

Closed

Fade in row header name

7179355

llorca reviewed Jan 11, 2017

View reviewed changes

michael-yx-wu added 9 commits January 11, 2017 15:42

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

giladgray suggested changes Jan 12, 2017

View reviewed changes

michael-yx-wu added 2 commits January 12, 2017 13:51

Clean up styles styles

dbe0801

Remove unused variables

1c99297

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

7ee1183

giladgray reviewed Jan 25, 2017

View reviewed changes

llorca reviewed Jan 25, 2017

View reviewed changes

Michael Wu added 2 commits January 25, 2017 14:04

Revert vscode setting changes

3cce68d

Clean up CSS

82c042e

michael-yx-wu mentioned this pull request Jan 25, 2017

Add table and column loading options #473

Merged

2 tasks

Use floor / ceil instead of custom padding

e2e0fdd

Rename vertical center mixin

b3277a4

Move vertical center mixin to cell/common

501c580

michael-yx-wu force-pushed the mw/table-header-loading branch from e7478c1 to 501c580 Compare January 25, 2017 22:19

Rename mixin

c44faf8

michael-yx-wu force-pushed the mw/table-header-loading branch from 9e59aa4 to c44faf8 Compare January 25, 2017 22:21

llorca reviewed Jan 25, 2017

View reviewed changes

giladgray suggested changes Jan 25, 2017

View reviewed changes

giladgray reviewed Jan 25, 2017

View reviewed changes

giladgray approved these changes Jan 25, 2017

View reviewed changes

Change header height to 8px

13bedbb

michael-yx-wu force-pushed the mw/table-header-loading branch from 22c337e to 13bedbb Compare January 25, 2017 23:39

gscshoyru merged commit 9077435 into master Jan 26, 2017

michael-yx-wu deleted the mw/table-header-loading branch January 26, 2017 18:54

giladgray mentioned this pull request Jan 27, 2017

Prepare Release 1.7.0 #563

Merged

Support loading prop on header cells #452

Support loading prop on header cells #452

Conversation

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

Checklist

blueprint-bot commented Jan 11, 2017

Update docs and example

blueprint-bot commented Jan 11, 2017

Fix css syntax bug

giladgray 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

blueprint-bot commented Jan 12, 2017

Shorten row header skeletons

llorca commented Jan 12, 2017 • edited Loading

giladgray 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

blueprint-bot commented Jan 12, 2017

Remove unused variables

blueprint-bot commented Jan 24, 2017

Add column header loading test

giladgray 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

giladgray Jan 25, 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

blueprint-bot commented Jan 25, 2017

Clean up CSS

blueprint-bot commented Jan 25, 2017

Use floor / ceil instead of custom padding

blueprint-bot commented Jan 25, 2017

Rename vertical center mixin

blueprint-bot commented Jan 25, 2017

Move vertical center mixin to cell/common

Choose a reason for hiding this comment

blueprint-bot commented Jan 25, 2017

Move vertical center mixin to cell/common

blueprint-bot commented Jan 25, 2017

Rename mixin

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 25, 2017

Change header height to 8px

Choose a reason for hiding this comment

blueprint-bot commented Jan 25, 2017

Change header height to 8px

michael-yx-wu commented Jan 25, 2017

michael-yx-wu commented Jan 11, 2017 •

edited

Loading

llorca commented Jan 12, 2017 •

edited

Loading

giladgray Jan 25, 2017 •

edited

Loading