feat(pfCard): Add a loading state to the pfCards #670

amarie401 · 2017-11-03T15:17:39Z

Description

Adding a loading state feature to the remaining pfCards as a continuation of PR #661. This would close #662

pfAggregateStatusCard
pfInfoStatusCard
pfCards - Trends (example)

PR Checklist

Unit tests are included
Screenshots are attached (if there are visual changes in the UI)
A Designer (@beanh66) is assigned as a reviewer (if there are visual changes in the UI)
A CSS rep (@cshinn) is assigned as a reviewer (if there are visual changes in the UI)

dtaylor113

LGTM. Nice job!

cshinn · 2017-11-03T18:49:20Z

@amarie401 what's the -22px position on the spinner container for? to compensate for default padding or something? If so, would it be possible to use a pf variable that corresponds to the thing that's being compensated for instead?

dtaylor113 · 2017-11-03T18:53:25Z

@amarie401 what's the -22px position on the spinner container for?

Hi @cshinn, when card is in the process of loading, it's body/contents are not shown, so there is no 'height' to the card. These changes add a height and adjust the position of the spinner.

jeff-phillips-18 · 2017-11-03T18:58:46Z

Wondering if, at minimum, we should all the consumer to set the card height when loading. When these cards are in a dashboard, the size changes may cause issues.

beanh66 · 2017-11-03T19:11:18Z

+1 @jeff-phillips-18 I was wondering the same thing, if it would be a problem when card heights all of a sudden change on a dashboard. I like your suggestion

dtaylor113 · 2017-11-03T19:21:29Z

Wondering if, at minimum, we should all the consumer to set the card height when loading

Ok, we could add a defaultCardHeight or a loadingCardHeight to the main status configuration obj.

amarie401 · 2017-11-06T20:52:47Z

I have added a loadingCardHeight option to the config for consumers to set the card height

AggStatus

InfoStatus

dtaylor113 · 2017-11-06T20:59:28Z

src/card/aggregate-status/aggregate-status-card.component.js

@@ -63,13 +66,19 @@
       <i>(depreciated, use layout = 'tall' instead)</i>
       </br></br>
       <pf-aggregate-status-card status="aggStatusAlt" show-top-border="true" alt-layout="true"></pf-aggregate-status-card>
+       <br/>
+       <label>Loading State</label>
+       <pf-aggregate-status-card status="aggStatusAlt2" loading-card-height="150px" show-top-border="true" show-spinner="dataLoading" spinner-text="Loading" layout="tall"></pf-aggregate-status-card>


Looking around at other examples of this in angular-patternfly we do not include the 'px' as part of the string, just the number.

Can remove :)

dtaylor113 · 2017-11-06T21:03:14Z

src/card/aggregate-status/aggregate-status-card.html

@@ -28,8 +35,8 @@ <h2 class="card-pf-title">
    </p>
  </div>
 </div>
-<div ng-if="$ctrl.isMiniLayout" class="card-pf card-pf-aggregate-status card-pf-aggregate-status-mini" ng-class="{'card-pf-accented': $ctrl.shouldShowTopBorder}">
-  <h2 class="card-pf-title">
+<div ng-if="$ctrl.isMiniLayout" class="card-pf card-pf-aggregate-status card-pf-aggregate-status-mini" ng-class="{'card-pf-accented': $ctrl.shouldShowTopBorder}" ng-style="$ctrl.spinnerHeight">


Not sure why the ng-style was added here? It seemed to work in the show-spinner div below.

dtaylor113 · 2017-11-06T21:04:18Z

src/card/aggregate-status/aggregate-status-card.html

@@ -41,7 +48,14 @@ <h2 class="card-pf-title">
      {{$ctrl.status.title}}
    </span>
  </h2>
-  <div class="card-pf-body">
+  <div class="card-pf-body" ng-class="{'show-spinner': $ctrl.showSpinner}">


Try moving the ng-style to here, it would be consistent with the !mini HTML above where we added the ng-style

dtaylor113 · 2017-11-06T21:16:27Z

Looking good, although the loading icon doesn't appear to be centered:

infoStatusCard:

aggregateStatusCard:

Seems like there is more space above the spinners than there is below the spinners.

amarie401 · 2017-11-06T21:21:48Z

Okay will look into it

jeff-phillips-18 · 2017-11-07T11:55:56Z

src/card/aggregate-status/aggregate-status-card.component.js

@@ -30,6 +30,9 @@
 *   </ul>
 * </ul>
 * @param {boolean=} show-top-border Show/hide the top border, true shows top border, false (default) hides top border
+ * @param {boolean=} showSpinner Show/Hide the spinner for loading state. True shows the spinner, false (default) hides the spinner
+ * @param {string=} spinnerText Text for the card spinner
+ * @param {string=} loadingCardHeight Height to set for the card when data is loading


We use loading here but showSpinner. I think it needs to be named such that it is obvious that this is the card height when showing the spinner.

good point. spinnerCardHeight?

I think renaming to spinnerCardHeight is good to show the correlation

jeff-phillips-18 · 2017-11-07T13:38:17Z

src/card/aggregate-status/aggregate-status-card.html

@@ -13,8 +13,15 @@ <h2 class="card-pf-title">
      <span class="card-pf-aggregate-status-title">{{$ctrl.status.title}}</span>
    </span>
  </h2>
-  <div class="card-pf-body">
-    <p class="card-pf-aggregate-status-notifications">
+  <div class="card-pf-body" ng-class="{'show-spinner': $ctrl.showSpinner}" ng-style="$ctrl.spinnerHeight">


the spinnerHeight ought only be used when the spinner is shown.

jeff-phillips-18 · 2017-11-07T13:38:58Z

src/card/aggregate-status/aggregate-status-card.html

@@ -29,7 +36,7 @@ <h2 class="card-pf-title">
  </div>
 </div>
 <div ng-if="$ctrl.isMiniLayout" class="card-pf card-pf-aggregate-status card-pf-aggregate-status-mini" ng-class="{'card-pf-accented': $ctrl.shouldShowTopBorder}">
-  <h2 class="card-pf-title">
+  <h2 class="card-pf-title" ng-class="{'hide-for-spinner': $ctrl.showSpinner}">


Should we be hiding the title while data is loading?

I believe the title is showing and not hidden with the base pfCard from my previous PR, I can change this to be consistent.

amarie401 · 2017-11-07T16:56:08Z

I have changed the name from loadingCardHeight to spinnerCardHeight for better correlation, unhid the title when loading and updated the ng-style to make the spinnerHeight more dynamic. @dtaylor113 are you still seeing the icon not being centered?

beanh66 · 2017-11-07T17:22:45Z

@amarie401 did you update the gif? I am still seeing that it isn't quite centered but I might be looking at an old version!

dtaylor113 · 2017-11-07T20:25:14Z

The info status card should likely show the title and the icon just not the status information.

Might the icon and/or title change based upon the loaded data? Ie. show an error icon/msg? If so, we should just hide everything until the data is loaded.

jeff-phillips-18 · 2017-11-07T20:28:03Z

The application can leave those values unset if that be the case. Can we try that scenario as well?

jeff-phillips-18 · 2017-11-07T20:29:15Z

My point being, that a blank card with just a spinner gives no indication of what is being loaded. If any of the info can be shown, then it gives the user an idea of what they are waiting for.

dtaylor113 · 2017-11-08T13:54:48Z

Went looking to find PF doc/examples/css/HTML for this card, I can't find InfoStatusCard in any of these locations:
https://github.com/patternfly/patternfly-design/tree/master/pattern-library/cards
http://www.patternfly.org/pattern-library/cards/base-card/
https://rawgit.com/patternfly/patternfly/master-dist/dist/tests/card-view-single-select.html

jeff-phillips-18 · 2017-11-08T15:14:31Z

@dtaylor113 The PR for this is still up in patternfly-design: patternfly/patternfly-design#483

dtaylor113 · 2017-11-08T16:39:24Z

@dtaylor113 The PR for this is still up in patternfly-design: patternfly/patternfly-design#483

ah...that's why I couldn't find it :-) So this seems to be a case of implementation before official PF html/css/less.

jeff-phillips-18 · 2017-11-08T17:54:55Z

So this seems to be a case of implementation before official PF html/css/less.

Yeah, feel free to implement in PF first 😉

dtaylor113 · 2017-11-08T18:57:55Z

Still not exactly centered. 'with HTML' card doesn't initially set icon or title. 'Loading State' card does initially set icon and title:

amarie401 · 2017-11-08T19:01:28Z

Does the icon image have any margins? Seems like the loading icon is being pushed over to the right. Could the right margin be removed/reduced during loading?

dtaylor113 · 2017-11-10T20:01:45Z

Hi, pushed latest changes. The infoStatusCard spinner icon is now centered within the card, with and without titles/icons:

beanh66

LGTM

jeff-phillips-18 · 2017-11-10T21:31:35Z

src/card/aggregate-status/aggregate-status-card.component.js

@@ -103,28 +112,49 @@
          "href":"#"
        }
      ]
-     };
+    };
+


Should setup the $scope.aggStatusAlt2 here with the title but the count and notifications being unknown.

jeff-phillips-18 · 2017-11-10T21:33:24Z

src/card/aggregate-status/aggregate-status-card.html

@@ -13,8 +13,15 @@ <h2 class="card-pf-title">
      <span class="card-pf-aggregate-status-title">{{$ctrl.status.title}}</span>
    </span>
  </h2>
-  <div class="card-pf-body">
-    <p class="card-pf-aggregate-status-notifications">
+  <div class="card-pf-body" ng-class="{'show-spinner': $ctrl.showSpinner}" ng-style="$ctrl.showSpinner ? $ctrl.spinnerHeight : {}">


I would expect the height to be set on the card, not on the body. It is called spinnerCardHeight and given the title height could potentially change based on knowing the title prior to data load it would be better to set the full card height.

jeff-phillips-18 · 2017-11-10T21:37:57Z

src/card/info-status/info-status-card.html

    <img ng-if="$ctrl.status.iconImage" ng-src="{{$ctrl.status.iconImage}}" alt=""
         class="info-img"/>
    <span class="info-icon {{$ctrl.status.iconClass}}"></span>
  </div>
-  <div class="card-pf-info-content">
+  <div class="card-pf-info-content card-pf-body" ng-class="{'show-spinner': $ctrl.showSpinner}" ng-style="$ctrl.showSpinner ? $ctrl.spinnerHeight : {}">


Again, the height should be on the entire card

dtaylor113 · 2017-11-13T16:16:25Z

src/card/examples/card-trend.js

 * @param {boolean=} showTopBorder Show/Hide the blue top border. True shows top border, false (default) hides top border
+ * @param {boolean=} showSpinner Show/Hide the spinner for loading state. True shows the spinner, false (default) hides the spinner
 * @param {boolean=} showTitlesSeparator Show/Hide the grey line between the title and sub-title.


Need to document spinnerCardHeight

amarie401 · 2017-11-13T19:51:40Z

With @jeff-phillips-18 changes

aggStatus:

infoStat:

jeff-phillips-18 · 2017-11-13T20:27:28Z

Not sure why your image above doesn't have the white background for the aggregate status cards. I pulled and tested and they correctly had the white background.

amarie401 · 2017-11-13T20:31:18Z

@jeff-phillips-18 I noticed that as well. The giphy recording tool I'm using is being a bit weird it's showing the correct background but when I upload it doesn't show it anymore... but the actual background is correct and how it should be :)

amarie401 requested review from jeff-phillips-18, dtaylor113 and beanh66 November 3, 2017 15:17

dtaylor113 previously approved these changes Nov 3, 2017

View reviewed changes

jeff-phillips-18 requested a review from cshinn November 3, 2017 18:28

cshinn previously approved these changes Nov 3, 2017

View reviewed changes

amarie401 dismissed stale reviews from cshinn and dtaylor113 via 7e977f5 November 6, 2017 15:37

amarie401 force-pushed the pf-cards-loading branch 4 times, most recently from 15e6107 to 8499e0b Compare November 6, 2017 20:49

dtaylor113 reviewed Nov 6, 2017

View reviewed changes

amarie401 force-pushed the pf-cards-loading branch from 8499e0b to 7fc5be4 Compare November 6, 2017 21:13

jeff-phillips-18 reviewed Nov 7, 2017

View reviewed changes

amarie401 force-pushed the pf-cards-loading branch from 7fc5be4 to 2ff39e8 Compare November 7, 2017 16:55

dtaylor113 dismissed beanh66’s stale review via 3950e79 November 8, 2017 18:54

dtaylor113 force-pushed the pf-cards-loading branch from 3950e79 to 91e07a2 Compare November 10, 2017 19:58

dtaylor113 previously approved these changes Nov 10, 2017

View reviewed changes

beanh66 previously approved these changes Nov 10, 2017

View reviewed changes

jeff-phillips-18 reviewed Nov 10, 2017

View reviewed changes

amarie401 dismissed stale reviews from beanh66 and dtaylor113 via 1dfcade November 13, 2017 15:10

dtaylor113 reviewed Nov 13, 2017

View reviewed changes

amarie401 force-pushed the pf-cards-loading branch from 1dfcade to 5edb266 Compare November 13, 2017 17:20

feat(pfCard): Add a loading state to the pfCards

c6b6f0f

amarie401 force-pushed the pf-cards-loading branch from 80ddc03 to c6b6f0f Compare November 13, 2017 19:04

jeff-phillips-18 approved these changes Nov 13, 2017

View reviewed changes

beanh66 approved these changes Nov 14, 2017

View reviewed changes

dtaylor113 approved these changes Nov 14, 2017

View reviewed changes

dtaylor113 merged commit 02054a8 into patternfly:master Nov 14, 2017

feat(pfCard): Add a loading state to the pfCards #670

feat(pfCard): Add a loading state to the pfCards #670

Uh oh!

Conversation

amarie401 commented Nov 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

PR Checklist

Uh oh!

dtaylor113 left a comment

Choose a reason for hiding this comment

Uh oh!

cshinn commented Nov 3, 2017

Uh oh!

dtaylor113 commented Nov 3, 2017

Uh oh!

jeff-phillips-18 commented Nov 3, 2017

Uh oh!

beanh66 commented Nov 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dtaylor113 commented Nov 3, 2017

Uh oh!

amarie401 commented Nov 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dtaylor113 commented Nov 6, 2017

Uh oh!

amarie401 commented Nov 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amarie401 commented Nov 7, 2017

Uh oh!

beanh66 commented Nov 7, 2017

Uh oh!

dtaylor113 commented Nov 7, 2017

Uh oh!

jeff-phillips-18 commented Nov 7, 2017

Uh oh!

jeff-phillips-18 commented Nov 7, 2017

Uh oh!

dtaylor113 commented Nov 8, 2017

Uh oh!

jeff-phillips-18 commented Nov 8, 2017

Uh oh!

dtaylor113 commented Nov 8, 2017

Uh oh!

jeff-phillips-18 commented Nov 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dtaylor113 commented Nov 8, 2017

Uh oh!

amarie401 commented Nov 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dtaylor113 commented Nov 10, 2017

Uh oh!

beanh66 left a comment

amarie401 commented Nov 3, 2017 •

edited

Loading

beanh66 commented Nov 3, 2017 •

edited

Loading

amarie401 commented Nov 6, 2017 •

edited

Loading

amarie401 commented Nov 6, 2017 •

edited

Loading

jeff-phillips-18 commented Nov 8, 2017 •

edited

Loading

amarie401 commented Nov 8, 2017 •

edited

Loading

amarie401 commented Nov 13, 2017 •

edited

Loading