feat(pfCard): Add loading state for pfCard #661

amarie401 · 2017-10-25T18:55:29Z

Description

Adding a loading state spinner to pfCard to fix #658

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)

@beanh66 @jennyhaines @dtaylor113 @jeff-phillips-18 @serenamarie125

coveralls · 2017-10-25T19:02:05Z

Coverage decreased (-0.4%) to 54.406% when pulling 82c92ca on amarie401:card-loading-state into c4193f1 on patternfly:master.

dtaylor113 · 2017-10-25T19:23:13Z

src/card/examples/card-timeframe.js

@@ -40,6 +41,9 @@
       <pf-card head-title="Card Title" sub-title="Card Subtitle" show-top-border="true" filter="filterConfigHeader" style="width: 50%">
         Card Contents
       </pf-card>
+     <label class="label-title">Timeframe filter in header & loading state</label>
+       <pf-card show-spinner="true" head-title="Card Title" sub-title="Card Subtitle" show-top-border="true" filter="filterConfigHeader" style="width: 50%">


Please tie this to a $scope var which is initially 'true'. Then in a $timeout delay, have it set to 'false' . Just so we can see the state change.

dtaylor113

Please tie this to a $scope var which is initially 'true'. Then in a $timeout delay, have it set to 'false' . Just so we can see the state change.

dtaylor113 · 2017-10-25T19:30:00Z

I'm wondering if we should also hide the filter dropdown (in header and/or footer) and the footer if the spinner is showing? If the data isn't available yet, we probably shouldn't show a filter control yet.

beanh66 · 2017-10-25T20:05:51Z

@dtaylor113 Agreed, I would think filters and actions should be hidden as well, thanks!

coveralls · 2017-10-25T20:11:35Z

Coverage decreased (-0.4%) to 54.406% when pulling 6d25b74 on amarie401:card-loading-state into c4193f1 on patternfly:master.

jeff-phillips-18 · 2017-10-25T20:13:15Z

src/card/basic/card.component.js

@@ -91,6 +92,7 @@ angular.module('patternfly.card').component('pfCard', {
    subTitle: '@?',
    showTopBorder: '@?',
    showTitlesSeparator: '@?',
+    showSpinner: '@?',


Better to make this a binding and use true/false rather that 'true'/'false'

jeff-phillips-18 · 2017-10-25T20:14:45Z

src/card/basic/card.component.js

@@ -130,6 +130,9 @@ angular.module('patternfly.card').component('pfCard', {

    ctrl.$onInit = function () {
      ctrl.shouldShowTitlesSeparator = (!ctrl.showTitlesSeparator || ctrl.showTitlesSeparator === 'true');
+      if (angular.isUndefined(ctrl.showSpinner)) {
+        ctrl.showSpinner = 'false';
+      }
    };


Once this is a binding you can use
ctrl.showSpinner = ctrl.showSpinner === true

@jeff-phillips-18 Shouldn't I do ctrl.showSpinner = ctrl.showSpinner === false if we want it to be false by default? Or should it be set to true by default? If the spinner showing is dependent on if there is data to load or not..

so you want it to be true if they pass false?

What I have above says it is true only if the specifically pass true. Any other value would resolve to false (the default).

jeff-phillips-18 · 2017-10-25T20:17:45Z

src/card/basic/card.html


  <div class="card-pf-body">
+    <div ng-class="{'spinner spinner-lg': $ctrl.showSpinner === 'true'}"></div>


This div should only exist if $ctrl.showSpinner

jeff-phillips-18 · 2017-10-25T20:17:47Z

src/card/basic/card.html


  <div class="card-pf-body">
+    <div ng-class="{'spinner spinner-lg': $ctrl.showSpinner === 'true'}"></div>
    <div ng-transclude></div>


This should not be shown if showSpinner is true. You may need to use ng-show though due to the transclude.

coveralls · 2017-10-25T20:38:29Z

Coverage decreased (-0.4%) to 54.406% when pulling 208898e on amarie401:card-loading-state into c4193f1 on patternfly:master.

dtaylor113 · 2017-10-25T20:59:34Z

src/card/examples/card-timeframe.js

@@ -45,10 +46,19 @@
          footer="footerConfig" filter="filterConfig" style="width: 50%">
        Card Contents
     </pf-card>
+     <label class="label-title">Loading State</label>
+     <pf-card show-spinner="dataLoading" head-title="Card Title" sub-title="Card Subtitle" show-top-border="true" filter="filterConfigHeader" style="width: 50%">


I'd add "Card Contents" here, between the <pf-card> and </pf-card>, just to show that transclusion will come through when the spinner is off.

jeff-phillips-18 · 2017-10-25T21:06:30Z

src/card/basic/card.component.js

@@ -130,6 +130,9 @@ angular.module('patternfly.card').component('pfCard', {

    ctrl.$onInit = function () {
      ctrl.shouldShowTitlesSeparator = (!ctrl.showTitlesSeparator || ctrl.showTitlesSeparator === 'true');
+      if (angular.isUndefined(ctrl.showSpinner)) {


No need to check if it is defined.

Okay no prob!

dtaylor113 · 2017-10-25T21:08:25Z

src/card/basic/card.html

@@ -1,15 +1,15 @@
 <div ng-class="$ctrl.showTopBorder === 'true' ? 'card-pf card-pf-accented' : 'card-pf'">
  <div ng-if="$ctrl.showHeader()" ng-class="$ctrl.shouldShowTitlesSeparator ? 'card-pf-heading' : 'card-pf-heading-no-bottom'">
-    <div ng-if="$ctrl.showFilterInHeader()" ng-include="'card/basic/card-filter.html'"></div>
+    <div ng-class="{'hide': $ctrl.showSpinner === true}" ng-if="$ctrl.showFilterInHeader()" ng-include="'card/basic/card-filter.html'"></div>


This might be better written as:
<div ng-if="$ctrl.showFilterInHeader() && !$ctrl.showSpinner" ...
...or you could even move the logic into the $ctrl.showFilterInHeader() function.

jeff-phillips-18 · 2017-10-25T21:08:35Z

src/card/basic/card.html


  <div class="card-pf-body">
+    <div ng-show="$ctrl.showSpinner" ng-class="{'spinner spinner-lg': $ctrl.showSpinner}"></div>


This should be ng-if and no need to ng-class since it won't be shown without the class.

dtaylor113 · 2017-10-25T21:09:29Z

src/card/basic/card.html

    <h2 class="card-pf-title">{{$ctrl.headTitle}}</h2>
  </div>
-
-  <span ng-if="$ctrl.subTitle" class="card-pf-subtitle">{{$ctrl.subTitle}}</span>
+  <span ng-class="{'hide': $ctrl.showSpinner === true}" ng-if="$ctrl.subTitle" class="card-pf-subtitle">{{$ctrl.subTitle}}</span>


I think the standard we use is ng-if instead of ng-class/hide, see comment above.

jeff-phillips-18 · 2017-10-25T21:09:31Z

src/card/basic/card.html


  <div class="card-pf-body">
+    <div ng-show="$ctrl.showSpinner" ng-class="{'spinner spinner-lg': $ctrl.showSpinner}"></div>
    <div ng-transclude></div>


This should be ng-show="!$ctrl.showSpinner"

dtaylor113 · 2017-10-25T21:09:35Z

src/card/basic/card.html

    <div ng-transclude></div>
  </div>
-  <div ng-if="$ctrl.footer" class="card-pf-footer">
+  <div ng-class="{'hide': $ctrl.showSpinner === true}" ng-if="$ctrl.footer" class="card-pf-footer">


I think the standard we use is ng-if instead of ng-class/hide, see comment above.

jeff-phillips-18 · 2017-10-25T21:10:10Z

src/card/basic/card.html

    <div ng-transclude></div>
  </div>
-  <div ng-if="$ctrl.footer" class="card-pf-footer">
+  <div ng-class="{'hide': $ctrl.showSpinner === true}" ng-if="$ctrl.footer" class="card-pf-footer">


I'd rather use ng-show than adding the 'hide' class

jeff-phillips-18 · 2017-10-25T21:10:37Z

src/card/basic/card.html

@@ -1,15 +1,15 @@
 <div ng-class="$ctrl.showTopBorder === 'true' ? 'card-pf card-pf-accented' : 'card-pf'">
  <div ng-if="$ctrl.showHeader()" ng-class="$ctrl.shouldShowTitlesSeparator ? 'card-pf-heading' : 'card-pf-heading-no-bottom'">
-    <div ng-if="$ctrl.showFilterInHeader()" ng-include="'card/basic/card-filter.html'"></div>
+    <div ng-class="{'hide': $ctrl.showSpinner === true}" ng-if="$ctrl.showFilterInHeader()" ng-include="'card/basic/card-filter.html'"></div>


ng-show rather than adding the 'hide' class?

Seems to be causing a lag with ng-if and ng-show.

coveralls · 2017-10-25T21:12:09Z

Coverage decreased (-0.4%) to 54.406% when pulling eae502d on amarie401:card-loading-state into c4193f1 on patternfly:master.

jeff-phillips-18 · 2017-10-25T21:12:57Z

test/card/basic/card.spec.js

+      cardClass = angular.element(element).find('.spinner-lg');
+      expect(cardClass.length).toBe(1);
+    });
+


Might be better to test the binding. Use show-spinner="dataLoading" then change the value and test again.

Sounds good. I'll go back to this

coveralls · 2017-10-25T21:28:49Z

Coverage increased (+0.008%) to 54.855% when pulling 9993661 on amarie401:card-loading-state into c4193f1 on patternfly:master.

amarie401 · 2017-10-25T21:55:24Z

After changing the ng-class to ng-if I noticed there is a bit of lag now and the icon is still there for a few seconds even after the data loads. I tried changing them to ng-show instead and the lag is still present. Not sure if this is a big issue or not @jeff-phillips-18 @dtaylor113

coveralls · 2017-10-25T22:37:37Z

Coverage decreased (-0.5%) to 54.381% when pulling 5f8039a on amarie401:card-loading-state into c4193f1 on patternfly:master.

serenamarie125 · 2017-10-26T01:57:42Z

Although I know in the issue uor guidance was to not have associated "Loading" text, I'm rethinking that decision.

Reasoning is that we have a similar question that just came in about a content view loading state - and in that situation, i think we need "Loading". Thoughts on adding it here for consistency with what we should do in a view as well as consistency with No Data Available state having an icon/label pair as well @beanh66 @jeff-phillips-18 @jennyhaines ?

beanh66 · 2017-10-26T13:50:25Z

@serenamarie125 that is fine with me. I was initially debating the label because I felt like it would be more consistent with the empty states we have. I can see where it is really necessary on larger views or cards. @jennyhaines would you be okay with that or would you rather see it optional maybe?

coveralls · 2017-10-26T13:51:25Z

Coverage decreased (-0.4%) to 54.4% when pulling e712c0c on amarie401:card-loading-state into c4193f1 on patternfly:master.

coveralls · 2017-10-26T20:19:16Z

Coverage decreased (-0.4%) to 54.4% when pulling 7c05269 on amarie401:card-loading-state into c4193f1 on patternfly:master.

jeff-phillips-18 · 2017-10-26T20:51:24Z

I believe this should be a feat rather than a fix.

amarie401 · 2017-10-26T20:56:12Z

No problem, will change to a feat.

coveralls · 2017-10-26T21:01:48Z

Coverage decreased (-0.4%) to 54.4% when pulling dfbec03 on amarie401:card-loading-state into c4193f1 on patternfly:master.

jeff-phillips-18 · 2017-10-26T21:02:10Z

@dtaylor113 The text should be to the right of the spinner. There are instances where the body content may not present enough vertical space to show the spinner and text stacked.

coveralls · 2017-10-26T21:02:36Z

Coverage decreased (-0.4%) to 54.4% when pulling dfbec03 on amarie401:card-loading-state into c4193f1 on patternfly:master.

beanh66 · 2017-10-26T21:23:35Z

@jeff-phillips-18 I was thinking it would be centered under the icon, similar to the empty state. Could we do that when there is enough space and when there is not, maybe just hide the text?

@serenamarie125 Thoughts?

jeff-phillips-18 · 2017-10-26T21:28:07Z

@beanh66 not sure we can tell if it will fit or not. I can't think of any way anyways.

jeff-phillips-18 · 2017-10-26T21:30:51Z

This is what we have now:

coveralls · 2017-10-26T21:35:24Z

Coverage decreased (-0.4%) to 54.436% when pulling b0292fc on amarie401:card-loading-state into c4193f1 on patternfly:master.

beanh66 · 2017-10-26T21:38:58Z

@jeff-phillips-18 Oh okay, got it. Well it looks a lot better being centered, I just didn't like the large separation between the icon/text originally :)

FWIW @serenamarie125 had MIQ take the ... out of their version so I would recommend just sticking with "Loading"

jeff-phillips-18 · 2017-10-26T21:42:28Z

The ... is just example text. Applications can do what they will.

coveralls · 2017-10-26T21:53:20Z

Coverage decreased (-0.4%) to 54.436% when pulling 3fb139e on amarie401:card-loading-state into c4193f1 on patternfly:master.

amarie401 · 2017-10-26T21:57:54Z

@beanh66 I have removed the "..." from the example text and changed it to "Loading" to be more consistent

beanh66 · 2017-10-26T22:01:22Z

Thanks, sorry for nit picking! 😆

coveralls · 2017-10-26T22:02:05Z

Coverage decreased (-0.4%) to 54.436% when pulling 658656f on amarie401:card-loading-state into c4193f1 on patternfly:master.