Add support for cell loading state #435

Merged
merged 35 commits into from Jan 10, 2017

Projects

None yet

6 participants

@michael-yx-wu
Collaborator
michael-yx-wu commented Jan 9, 2017 edited

Checklist

  • Include tests
  • Update documentation

Reviewers should focus on:

  • Are docs sufficient / good?
  • The example is located in the Table > Loading States > Cell section
michael-yx-wu added some commits Jan 6, 2017
@michael-yx-wu michael-yx-wu Add support for cell loading state d799760
@michael-yx-wu michael-yx-wu Add cell loading example
85cc286
@@ -244,7 +244,9 @@
}
#{section-id("components.table-js")} {
- .kss-example {
@michael-yx-wu
michael-yx-wu Jan 9, 2017 Collaborator

instead of modifying this class, we can just modify the bp-table-container class directly with the same effect

michael-yx-wu added some commits Jan 9, 2017
@michael-yx-wu michael-yx-wu Add cell test
aff3d2b
@michael-yx-wu michael-yx-wu Disable require rule on cell loading example
68211a0
@blueprint-bot
Collaborator

Disable require rule on cell loading example

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

michael-yx-wu added some commits Jan 9, 2017
@michael-yx-wu michael-yx-wu Inline skeleton width styles
857a827
@michael-yx-wu michael-yx-wu Remove random configuration from cell example
f69206d
@michael-yx-wu michael-yx-wu Increase loadable content skeleton range
94ea779
@adidahiya
Member

can you add an example of this feature to the table preview page as well?

also the latest docs preview doesn't work; webpack complains about a missing module in https://992-71939872-gh.circle-artifacts.com/0/home/ubuntu/blueprint/packages/docs/dist/index.html

@michael-yx-wu michael-yx-wu Add key to Columns
0a1482a
packages/table/src/_docs.scss
@@ -124,6 +124,28 @@ Styleguide components.table-js.example-editable
*/
/*
+Loading States
@llorca
llorca Jan 9, 2017 Contributor

Loading states, we're all sentence casing now 🔨

@michael-yx-wu
michael-yx-wu Jan 9, 2017 Collaborator

Oops!

packages/table/src/cell/_common.scss
@@ -18,7 +18,12 @@ $dark-cell-background-color: $dark-gray3 !default;
$dark-cell-border-color: $pt-dark-divider-black !default;
$dark-cell-text-color: $pt-dark-text-color !default;
+@mixin fade-in() {
@llorca
llorca Jan 9, 2017 Contributor

Is this worth a mixin? It's so short

@michael-yx-wu
michael-yx-wu Jan 9, 2017 Collaborator

I think it is. I use it in a couple places in the original PR.

packages/table/src/cell/cell.tsx
@@ -25,13 +27,28 @@ export interface ICellProps extends IIntentProps, IProps {
export type ICellRenderer = (rowIndex: number, columnIndex: number) => React.ReactElement<ICellProps>;
export const emptyCellRenderer = (_rowIndex: number, _columnIndex: number) => <Cell />;
+export const loadingCellRenderer = () => <Cell loading={true} />;
+
+export const CELL_CLASSNAME = "bp-table-cell";
@llorca
llorca Jan 9, 2017 Contributor

That should go into some kind of Classes.ts file

@michael-yx-wu
michael-yx-wu Jan 9, 2017 Collaborator

Sure. I'm going to make a new Classes.ts file in the Table package then

@giladgray
giladgray Jan 10, 2017 Member

tracked in #438

@michael-yx-wu
Collaborator

@adidahiya This is the first part of the table loading feature. Future PR(s) will include supporting showing loading states on individual EditableCells, ColumnHeaderCells, and RowHeaders. There will also be another PR that allows you to set "bulk" options to show loading states on all column headers, row headers, and table body cells.

Should all of these use cases get their own examples as well?

@adidahiya
Member

Feel free to split things up into small PRs; that is preferred. I don't think you need to go overboard with granular examples for every possible configuration.

@michael-yx-wu michael-yx-wu Add semicolon
e483c30
@michael-yx-wu
Collaborator

Okay. I'll add an example to the preview when I make the PR for the "bulk" loading options on the Table component itself.

@blueprint-bot
Collaborator

Add semicolon

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

michael-yx-wu added some commits Jan 9, 2017
@michael-yx-wu michael-yx-wu Add option for hiding all cell loading states
5b912e0
@michael-yx-wu michael-yx-wu Fade in cell loading state
8e967ef
@michael-yx-wu
Collaborator

screen shot 2017-01-09 at 2 31 41 pm

@llorca noticed that on Chrome the border-radius doesn't always seems render properly. This seems to be related to odd height and percentage width. Has anyone seen this before? @giladgray @adidahiya

michael-yx-wu added some commits Jan 9, 2017
@michael-yx-wu michael-yx-wu Stop overriding skeleton glow animation
402b323
@michael-yx-wu michael-yx-wu Ignore stylelint no unknown animation warning
32edcd7
@michael-yx-wu michael-yx-wu Lift up skeleton variables and animations
5109522
@michael-yx-wu michael-yx-wu Simplify cell-loading mixin
2fe1eb1
@michael-yx-wu michael-yx-wu Make cellTests test expect a value
0db754b
@michael-yx-wu michael-yx-wu Add loadableContent tests
6a46710
@blueprint-bot
Collaborator

Add loadableContent tests

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

packages/core/src/common/_progress.scss
@@ -0,0 +1,23 @@
+// Copyright 2017 Palantir Technologies, Inc. All rights reserved.
@giladgray
giladgray Jan 10, 2017 Member

this file should be progress/_common.scss

packages/core/src/common/classes.ts
@@ -63,6 +63,8 @@ export const INPUT_GROUP = "pt-input-group";
export const LABEL = "pt-label";
+export const LOADING = "pt-loading";
@giladgray
giladgray Jan 10, 2017 Member

this belongs in the "modifiers" block at the top, with ACTIVE

- background-color: $skeleton-color-end;
- }
-}
+@import "../../common/progress";
@giladgray
giladgray Jan 10, 2017 Member

update path

packages/table/src/cell/_common.scss
@@ -18,7 +18,12 @@ $dark-cell-background-color: $dark-gray3 !default;
$dark-cell-border-color: $pt-dark-divider-black !default;
$dark-cell-text-color: $pt-dark-text-color !default;
+@mixin text-fade-in() {
+ transition: color $pt-transition-duration * 3;
@giladgray
giladgray Jan 10, 2017 Member

this probably works better as a variable? $text-fade-in-transition

packages/table/src/cell/cell.tsx
@@ -25,13 +27,28 @@ export interface ICellProps extends IIntentProps, IProps {
export type ICellRenderer = (rowIndex: number, columnIndex: number) => React.ReactElement<ICellProps>;
export const emptyCellRenderer = (_rowIndex: number, _columnIndex: number) => <Cell />;
+export const loadingCellRenderer = () => <Cell loading={true} />;
+
+export const CELL_CLASSNAME = "bp-table-cell";
@giladgray
giladgray Jan 10, 2017 Member

tracked in #438

packages/table/src/cell/cell.tsx
+ Classes.intentClass(this.props.intent),
+ {
+ [Classes.LOADING]: loading,
+ },
@giladgray
giladgray Jan 10, 2017 Member

style nit: i typically inline one-prop objects. { [key]: val }

+
+import { Classes } from "@blueprintjs/core";
+
+export interface ILoadable {
@giladgray
giladgray Jan 10, 2017 Member

ILoadingProps for consistency with IIntentProps, etc?

+
+export interface ILoadable {
+ /**
+ * Render the loading state.
@giladgray
giladgray Jan 10, 2017 Member

not great docs...

+
+export interface ILoadableContentProps extends ILoadable {
+ /**
+ * Show a variable length skeleton when rendering the loading state.
@giladgray
giladgray Jan 10, 2017 Member

explain what "variable length" means here (using word in its own definition).
something about choosing a random width

+ public constructor(props: ILoadableContentProps) {
+ super(props);
+ this.skeletonLength = props.variableLength ? 75 - Math.floor(Math.random() * 11) * 5 : 100;
+ this.style = { width: `${this.skeletonLength}%` };
@giladgray
giladgray Jan 10, 2017 Member

store either skeletonLength or style cuz one is easily derivable from the other. your choice.

+
+ public render() {
+ if (this.props.loading) {
+ return <div className={`${Classes.SKELETON}`} style={this.style} />;
@giladgray
giladgray Jan 10, 2017 Member

no need to interpolate the one string. className={Classes.SKELETON}

+ return <div className={`${Classes.SKELETON}`} style={this.style} />;
+ }
+
+ return React.Children.only(this.props.children);
@giladgray
giladgray Jan 10, 2017 Member

this is probably worth documenting in the usage of this component: it requires exactly one child.

@giladgray
giladgray Jan 10, 2017 Member

also that it doesn't support string content.

though we could make it support string content, that might be valuable (see popover line 268).

packages/table/test/cellTests.tsx
@@ -30,6 +30,12 @@ describe("Cell", () => {
expect(cell.find(".inner").text()).to.equal("Purple");
});
+ it("renders loading state", () => {
+ const cellHarness = harness.mount( <Cell loading={true} />);
@giladgray
giladgray Jan 10, 2017 Member

remove leading whitespace: mount(<Cell

+ expect(loadableContentHarness.element.textContent).to.equal(someText);
+ });
+
+ it("does not render child when loading", () => {
@giladgray
giladgray Jan 10, 2017 Member

"renders skeleton instead of child when loading"

@@ -0,0 +1,42 @@
+// Copyright 2017 Palantir Technologies, Inc. All rights reserved.
@giladgray
giladgray Jan 10, 2017 Member

@themadcreator what do you think of this file living in common/ dir?
it's not a true common file because it defines CSS rules (not just sass stuff). but it affects both cells and headers.

@themadcreator
themadcreator Jan 10, 2017 Contributor

I suppose the loading stuff living here is okay. Maybe a separate subdirectory should be considered as well.

As far as the SASS, I think i would prefer to make the rules below a mixin and then have bp-table-cell and bp-table-header mix them in in their respective .scss files.

packages/table/src/common/_loading.scss
+@import "~@blueprintjs/core/src/common/progress";
+@import "~@blueprintjs/core/src/common/variables";
+
+@mixin cell-loading() {
@giladgray
giladgray Jan 10, 2017 Member

use this mixin directly in block below beginning line 31. could just inline it.

michael-yx-wu added some commits Jan 10, 2017
@michael-yx-wu michael-yx-wu Add tests, move scss around, respond to nits
6979248
@michael-yx-wu michael-yx-wu Merge remote-tracking branch 'origin/master' into mw/cell-loading
07d4b8e
@blueprint-bot
Collaborator

Merge remote-tracking branch 'origin/master' into mw/cell-loading

Preview: docs | table
Coverage: core | datetime

@giladgray

getting there but awkward usage of sass common files

@@ -9,3 +9,20 @@ $progress-track-color: rgba($gray1, 0.2) !default;
$progress-head-color: rgba($gray1, 0.8) !default;
$dark-progress-track-color: rgba($black, 0.5) !default;
$dark-progress-head-color: $gray3 !default;
+
+$skeleton-animation-duration: $pt-transition-duration * 20 !default;
@giladgray
giladgray Jan 10, 2017 Member

i'd refactor to make this even more easily reusable (and then you won't have to stylelint-disable):

$skeleton-animation: ($pt-transition-duration * 20) linear infinite glow !default;
@michael-yx-wu
michael-yx-wu Jan 10, 2017 Collaborator

nice! that's a good idea

+
+$skeleton-animation-duration: $pt-transition-duration * 20 !default;
+$skeleton-color-start: rgba($gray4, 0.2) !default;
+$skeleton-color-end: rgba($gray1, 0.2) !default;
@giladgray
giladgray Jan 10, 2017 Member

these vars don't belong here because they start with skeleton-. perhaps skeleton/_common.scss

+$skeleton-color-start: rgba($gray4, 0.2) !default;
+$skeleton-color-end: rgba($gray1, 0.2) !default;
+
+@keyframes glow {
@giladgray
giladgray Jan 10, 2017 Member

this is not kosher for a _common file. will be rendered to CSS twice. i would define this directly in _skeleton.scss

+import { Cell, Column, Table } from "../src";
+
+// tslint:disable-next-line:no-var-requires
+const bigSpaceRocks = require("./pha.json") as IBigSpaceRock[];
@giladgray
giladgray Jan 10, 2017 Member
  1. awkward to use interface before it's defined.
  2. can we give the file a better name? what is "pha?"
@michael-yx-wu
michael-yx-wu Jan 10, 2017 Collaborator
  1. okay, will reorder lines
  2. ripped this from slate docs. "potentially hazardous asteroids"
@giladgray
giladgray Jan 10, 2017 edited Member

lol sold on the acronym! 🚢

packages/table/src/cell/_common.scss
@@ -18,12 +18,15 @@ $dark-cell-background-color: $dark-gray3 !default;
$dark-cell-border-color: $pt-dark-divider-black !default;
$dark-cell-text-color: $pt-dark-text-color !default;
+$text-fade-in-transition: color $pt-transition-duration * 3;
@giladgray
giladgray Jan 10, 2017 Member

hrmm i take it back. make the variable be just the -duration and add !default to the end.

@michael-yx-wu
michael-yx-wu Jan 10, 2017 Collaborator

hm in that case i'd like to use this variable inside _loading.scss as well, though it feels weird to have that file import from this one

packages/table/src/cell/cell.tsx
@@ -25,13 +27,28 @@ export interface ICellProps extends IIntentProps, IProps {
export type ICellRenderer = (rowIndex: number, columnIndex: number) => React.ReactElement<ICellProps>;
export const emptyCellRenderer = (_rowIndex: number, _columnIndex: number) => <Cell />;
+export const loadingCellRenderer = () => <Cell loading={true} />;
@giladgray
giladgray Jan 10, 2017 Member

this guy's still here?

@michael-yx-wu
michael-yx-wu Jan 10, 2017 Collaborator

oops, removing

packages/table/src/common/_loading.scss
+// Copyright 2017 Palantir Technologies, Inc. All rights reserved.
+
+@import "~@blueprintjs/core/src/common/variables";
+@import "~@blueprintjs/core/src/components/progress/common";
@giladgray
giladgray Jan 10, 2017 Member

should import skeleton/common

+ expect(loadableContentHarness.element.textContent).to.equal(someText);
+ });
+
+ it("does not render string content", () => {
@giladgray
giladgray Jan 10, 2017 Member

not simply "does not render," but "throws error on string content"

same with next test

@blueprint-bot
Collaborator

Use more explicit test names

Preview: docs | table
Coverage: core | datetime

packages/table/src/cell/_common.scss
@@ -18,12 +18,15 @@ $dark-cell-background-color: $dark-gray3 !default;
$dark-cell-border-color: $pt-dark-divider-black !default;
$dark-cell-text-color: $pt-dark-text-color !default;
+$fade-in-duration: $pt-transition-duration * 3;
@llorca
llorca Jan 10, 2017 Contributor

$cell-transition-duration, or even abstract it all to $cell-transition

michael-yx-wu added some commits Jan 10, 2017
@michael-yx-wu michael-yx-wu Use more generic cell transition sass var name
25bcc39
@michael-yx-wu michael-yx-wu Add more entires to pHA.json
f0d78b4
@michael-yx-wu michael-yx-wu Start glowing animations after fade in completes
1d9d8a1
@michael-yx-wu michael-yx-wu Add a "random" option to the example
9acb4a2
@michael-yx-wu michael-yx-wu Recalculate cell skeleton style when necessary
f8f769d
@blueprint-bot
Collaborator

Recalculate cell skeleton style when necessary

Preview: docs | table
Coverage: core | datetime

+ * If true, show a fixed height loading skeleton instead of the cell content.
+ * @default false
+ */
+ loading?: boolean;
@michael-yx-wu
michael-yx-wu Jan 10, 2017 Collaborator

Do docs here need to be more elaborate? Note that these docs will be repeated by any component props that extends this interface

@themadcreator
themadcreator Jan 10, 2017 Contributor

This description seems fine to me

@giladgray
giladgray Jan 10, 2017 Member

if this interface will be shared across several components then the docs should be generalized so they can be reused in different contexts (for instance, don't talk about fixed-height or cells).

or, just define this prop separately in each interface with more complete and relevant documentation.

+
+ public componentWillReceiveProps(nextProps: ILoadableContentProps) {
+ if (!this.props.loading && nextProps.loading || this.props.variableLength !== nextProps.variableLength) {
+ this.calculateStyle(nextProps.variableLength);
@themadcreator
themadcreator Jan 10, 2017 Contributor

Since you use Math.random() every time in calculateStyle, won't this make the skeleton rectangle change size if the loading state switches back and forth?

@michael-yx-wu
michael-yx-wu Jan 10, 2017 Collaborator

Yes, that's the intended behavior. It seem weird to have rectangles be the same the random length when switching.

packages/table/src/common/_loading.scss
+ .pt-skeleton {
+ opacity: 0;
+ height: $pt-grid-size / 2;
+ animation: skeleton-animation($cell-transition-duration * 2),
@giladgray
giladgray Jan 10, 2017 Member

set animation-delay: 0 ($cell-transition-duration * 2); separately. also swap order so un-delayed transition is first (time order).

+$skeleton-color-end: rgba($gray1, 0.2) !default;
+
+@function skeleton-animation($delay: 0) {
+ @return ($pt-transition-duration * 20) linear infinite $delay glow;
@giladgray
giladgray Jan 10, 2017 Member

remove this function and just set animation-delay separately when necessary.

+ absoluteMagnitude: number;
+ discoverer: string;
+ observatory: string;
+ [key: string]: string | number;
@giladgray
giladgray Jan 10, 2017 Member

this doesn't seem necessary. there are no arbitrary keys.

@michael-yx-wu
michael-yx-wu Jan 10, 2017 Collaborator

removing, but in newer version of tsc you'll see this error:

message: 'Index signature of object type implicitly has an 'any' type.'
@giladgray
giladgray Jan 10, 2017 Member

should be safe to remove this line

+ * If true, show a fixed height loading skeleton instead of the cell content.
+ * @default false
+ */
+ loading?: boolean;
@giladgray
giladgray Jan 10, 2017 Member

if this interface will be shared across several components then the docs should be generalized so they can be reused in different contexts (for instance, don't talk about fixed-height or cells).

or, just define this prop separately in each interface with more complete and relevant documentation.

michael-yx-wu added some commits Jan 10, 2017
@michael-yx-wu michael-yx-wu Use animation-delay shorthand 01873f3
@michael-yx-wu michael-yx-wu Stop randomizing on render
2dc266a
@blueprint-bot
Collaborator

Stop randomizing on render

Preview: docs | table
Coverage: core | datetime

packages/table/src/_docs.scss
@@ -124,6 +124,30 @@ Styleguide components.table-js.example-editable
*/
/*
+Loading states
+
+When fetching or updating data, it may be desirable to show a loading state.
@giladgray
giladgray Jan 10, 2017 Member

add:

The Table components provide fine-grain loading control of loading row headers, column headers, or individual cells.

packages/table/src/_docs.scss
+Cell
+
+Although the most common use cases will probably involve showing the loading state on full columns,
+full rows, or the entire body, `Cell`s expose a `loading` prop for granular control of which cells
@giladgray
giladgray Jan 10, 2017 Member

remove everything before "Cells expose..."

packages/table/src/_docs.scss
+full rows, or the entire body, `Cell`s expose a `loading` prop for granular control of which cells
+should show a loading state. Below is a table of the largest potentially hazardous asteroid
+(based on absolute magnitude) discovered in a given year. Try selecting a different preset
+configuration.
@giladgray
giladgray Jan 10, 2017 Member

"preset loading configuration"

packages/table/src/common/_loading.scss
- animation: skeleton-animation($cell-transition-duration * 2),
- $cell-transition-duration linear forwards skeleton-fade-in;
+ animation: $skeleton-animation, $cell-transition-duration linear forwards skeleton-fade-in;
+ animation-delay: $cell-transition-duration, 0;
@giladgray
giladgray Jan 10, 2017 Member

i would flip the order so the 0-delay transition is first. then it reads in the order it renders.

michael-yx-wu added some commits Jan 10, 2017
@michael-yx-wu michael-yx-wu Expand ILoadingProps docs 2482c9a
@michael-yx-wu michael-yx-wu Flip animation list order to match execution order
735656c
@michael-yx-wu michael-yx-wu Streamline IBigSpaceRock
233ad33
@blueprint-bot
Collaborator

Streamline IBigSpaceRock

Preview: docs | table
Coverage: core | datetime

+ * If true, show the component's loading state. When set on a component that supports
+ * `ICellProps`, content will be hidden behind a fixed-height skeleton. When set on all other
+ * components, children components that support `ICellProps` will display their loading state.
+ * @default false
@michael-yx-wu
michael-yx-wu Jan 10, 2017 Collaborator

This interface will only be used on components that fall into these two categories. But having this indiscriminately in the docs feels weird. Better to just remove this then?

@michael-yx-wu
michael-yx-wu Jan 10, 2017 Collaborator

*and put directly in the props?

@michael-yx-wu michael-yx-wu Remove ILoadingProps
31fc756
@blueprint-bot
Collaborator

Remove ILoadingProps

Preview: docs | table
Coverage: core | datetime

@giladgray giladgray update cell loading docs
1afac1d
@blueprint-bot
Collaborator

update cell loading docs

Preview: docs | table
Coverage: core | datetime

@giladgray giladgray merged commit b1b5e77 into master Jan 10, 2017

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
cla/palantir CLA signed via membership in "palantir"
Details
@giladgray giladgray referenced this pull request Jan 13, 2017
Merged

Prepare Release v1.6.0 #487

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment