Alternate Label Placement for TrendsCard & TrendsChart #594

amarie401 · 2017-08-24T14:03:55Z

Description

Support for new layout for trend card & trend chart with label position on the left (default) or right for compact layout. Added new config option for label position. This would close #580

TrendsChart:
https://rawgit.com/amarie401/angular-patternfly/feat-trend-card-dist/dist/docs/index.html#/api/patternfly.charts.directive:pfTrendsChart

TrendsCard:
https://rawgit.com/amarie401/angular-patternfly/feat-trend-card-dist/dist/docs/index.html#/api/patternfly.card.component:pfCard%20-%20Trends

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)

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

amarie401 · 2017-08-24T14:15:36Z

I believe there needs to be more spacing between the chart and details for the right label position but was having a little trouble manipulating the spacing with CSS. Any suggestions?

jeff-phillips-18 · 2017-08-24T14:30:01Z

@amarie401 Look at the updates being done for #591 , this will effect your changes.

beanh66 · 2017-08-24T19:32:01Z

@amarie401 do you have the rawgit link for this so we can review the interaction!?

amarie401 · 2017-08-24T20:03:54Z

@beanh66 whoops! here it is :)

https://rawgit.com/amarie401/angular-patternfly/feat-trend-card-dist/dist/docs/index.html#/api/patternfly.charts.directive:pfTrendsChart

https://rawgit.com/amarie401/angular-patternfly/feat-trend-card-dist/dist/docs/index.html#/api/patternfly.card.component:pfCard%20-%20Trends

beanh66 · 2017-08-24T20:14:39Z

Thanks @amarie401! Looks great to me. Just wondering two things:
1 - Is this spacing bug being addressed outside of this PR (I assume yes)
2 - Should the radio button option for putting the value on the right instead of left also be available when looking at the "Inline" option? I noticed it only appears with the "Compact" option as of now, but just curious!

amarie401 · 2017-08-24T20:30:37Z

@beanh66 in #580 @dtaylor113 suggested adding it to the compact layout because it was very close to what @serenamarie125 had provided in her issue. If it should also be in the "inline" option then I can add it there as well if you want & yes in #591 it is being addressed! :)

amarie401 · 2017-08-25T13:19:10Z

@jeff-phillips-18 @beanh66 It seems that the spacing issue that @dtaylor113 is working on will only affect the spacing for the default "left" placement. I will have to implement something similar for the "right" alternate placement.

beanh66 · 2017-08-25T13:59:54Z

@amarie401 That works for me! I'll leave it up to @serenamarie125 to decide if we need this in the "inline" option too!

cdcabrera · 2017-08-25T15:33:19Z

src/charts/trends/trends-chart.html

@@ -19,7 +19,7 @@
  </div>
  <div ng-switch-when="compact" class="trend-card-compact-pf">
    <div class="row trend-row">
-      <div class="col-sm-4 col-md-4">
+      <div class="col-sm-4 col-md-4" ng-class="{'col-sm-push-8 col-md-push-8': $ctrl.config.compactLabelPosition === 'right'}">


@amarie401 just a callout... as discovered this is going to conflict with @dtaylor113 PR...

cdcabrera · 2017-08-25T15:33:43Z

src/charts/trends/trends-chart.html

@@ -32,7 +32,7 @@
          <span class="trend-header-compact-pf" ng-if="$ctrl.config.title">{{$ctrl.config.title}}</span>
        </div>
      </div>
-      <div class="col-sm-8 col-md-8">
+      <div class="col-sm-8 col-md-8" ng-class="{'col-sm-pull-4 col-md-pull-4': $ctrl.config.compactLabelPosition === 'right'}">


same callout on the conflict...

cdcabrera · 2017-08-25T15:34:24Z

src/card/examples/card-trend.js

@@ -81,9 +84,19 @@
         'layout'       : 'compact',
         'valueType'    : 'actual',
         'units'        : 'TB',
-         'tooltipType'  : 'percentage'
+         'tooltipType'  : 'percentage',


oh the trailing comma... though in newer ECMA this is acceptable oddly

cdcabrera · 2017-08-25T15:35:26Z

src/charts/trends/trends-chart.component.js

@@ -101,7 +102,7 @@
         </div>
       </div>
       <div class="row">
-         <div class="col-md-6">
+         <div class="col-md-4">
           <form role="form"">


oh the double quote quote... was already there, could fix if you want, happened to me too... might want to do a search on the page for any remaining ones

cdcabrera · 2017-08-25T15:35:51Z

src/charts/trends/trends-chart.component.js

@@ -110,6 +111,19 @@
             </div>
           </form>
         </div>
+         <div class="col-md-4" ng-if="config.layout === 'compact'">
+          <form role="form"">


double quote quote

cdcabrera · 2017-08-25T15:36:50Z

test/charts/trends/trends-chart.spec.js

+    expect(trendCard.hasClass('.col-sm-push-8 .col-md-push-8'));
+    trendCard = element.find('.col-sm-8 .col-md-8');
+    expect(trendCard.hasClass('.col-sm-pull-4 .col-md-pull-4'));
+  });


with @dtaylor113 changes to the classes, combined with yours you'll probably have to fix these

cdcabrera · 2017-08-25T16:31:15Z

test/charts/trends/trends-chart.spec.js

+    trendCard = element.find('.col-sm-4 .col-md-4');
+    expect(trendCard.hasClass('.col-sm-push-8 .col-md-push-8'));
+    trendCard = element.find('.col-sm-8 .col-md-8');
+    expect(trendCard.hasClass('.col-sm-pull-4 .col-md-pull-4'));


as discussed unit test missing methods...

expect(SOMETHING).toEqual(SOMETHING)

beanh66 · 2017-08-25T18:54:53Z

@amarie401 oops I think I looked at the wrong thing or didn't look closely. The value IS already on the right side for the "inline" case :) Sorry!

beanh66

LGTM!

serenamarie125

LGTM @amarie401 !

jeff-phillips-18 · 2017-08-25T19:13:04Z

Is the rawgit link updated?

amarie401 · 2017-08-25T19:16:48Z

@jeff-phillips-18 it should be. I will update the photos in the PR as well

jeff-phillips-18 · 2017-08-25T19:22:05Z

src/charts/charts.less

@@ -67,7 +67,7 @@ pf-c3-chart {
  .col-sm-2 {
    padding-right: 0;
  }
-  .col-sm-10 {
+  .col-sm-10:not(.col-sm-pull-2) {
    padding-left: 0;
  }


I would have expected this check to be on both columns and to do the reverse padding on them. That is, whichever column ends up on the left has a right-padding 0, and whichever ends up on the right has a left-padding of 0.

@jeff-phillips-18 If I change the styling to this then the details is too close to the chart for the right label position as shown below unless I remove the .col-sm-2.col-sm-push-10 { padding-left: 0; } then the spacing is fine. Should I leave that specific style out?

.trend-card-compact-pf { .col-sm-2:not(.col-sm-push-10) { padding-right: 0; } .col-sm-2.col-sm-push-10 { padding-left: 0; } .col-sm-10:not(.col-sm-pull-2) { padding-left: 0; } .col-sm-10.col-sm-pull-2 { padding-right: 0; } }

@jeff-phillips-18 what are your thoughts?

I guess I was assuming the text would be right justified in this layout. If not, then I guess we should leave the spacing as it was.

jeff-phillips-18 · 2017-08-28T12:52:06Z

Looking at the design from http://www.patternfly.org/pattern-library/cards/trend-card/ it doesn't appear that it should be right justified, so my mistake. Sorry for the confusion.

amarie401 · 2017-08-28T12:53:22Z

No worries! @jeff-phillips-18

amarie401 · 2017-08-28T14:01:24Z

@jeff-phillips-18 @cdcabrera please review

cdcabrera

Minor fix, then should be good for my aspect

cdcabrera · 2017-08-28T14:11:07Z

src/charts/trends/trends-chart.component.js

+           </label>
+           </div>
+           </form>
+           </div>


Indentation so it matches the other examples...

dtaylor113

Hi, cloned this branch. Trend chart and card LGTM. -thanks

label config

beanh66 mentioned this pull request Aug 24, 2017

Adding a trend card variation for values on the right patternfly/patternfly-design#374

Merged

6 tasks

amarie401 requested review from beanh66 and cshinn August 24, 2017 16:48

amarie401 force-pushed the feat-trend-card branch from f20a02d to 02c487f Compare August 24, 2017 19:48

amarie401 requested review from jeff-phillips-18, dtaylor113 and cdcabrera August 25, 2017 14:29

cdcabrera requested changes Aug 25, 2017

View reviewed changes

cdcabrera reviewed Aug 25, 2017

View reviewed changes

amarie401 force-pushed the feat-trend-card branch 4 times, most recently from d9dfa02 to fcd8e6c Compare August 25, 2017 18:42

beanh66 approved these changes Aug 25, 2017

View reviewed changes

serenamarie125 approved these changes Aug 25, 2017

View reviewed changes

jeff-phillips-18 requested changes Aug 25, 2017

View reviewed changes

amarie401 force-pushed the feat-trend-card branch from fcd8e6c to e30bba8 Compare August 28, 2017 14:00

cdcabrera requested changes Aug 28, 2017

View reviewed changes

cdcabrera reviewed Aug 28, 2017

View reviewed changes

dtaylor113 approved these changes Aug 28, 2017

View reviewed changes

fix(pfTrendsChart): Updated trend chart and trend card with right

afe1aa4

label config

amarie401 force-pushed the feat-trend-card branch from e30bba8 to afe1aa4 Compare August 28, 2017 14:16

cdcabrera approved these changes Aug 28, 2017

View reviewed changes

jeff-phillips-18 approved these changes Aug 28, 2017

View reviewed changes

cdcabrera merged commit 4207e65 into patternfly:master Aug 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Alternate Label Placement for TrendsCard & TrendsChart #594

Alternate Label Placement for TrendsCard & TrendsChart #594

amarie401 commented Aug 24, 2017 •

edited

Loading

amarie401 commented Aug 24, 2017 •

edited

Loading

jeff-phillips-18 commented Aug 24, 2017

beanh66 commented Aug 24, 2017

amarie401 commented Aug 24, 2017 •

edited

Loading

beanh66 commented Aug 24, 2017 •

edited

Loading

amarie401 commented Aug 24, 2017 •

edited

Loading

amarie401 commented Aug 25, 2017 •

edited

Loading

beanh66 commented Aug 25, 2017

cdcabrera Aug 25, 2017

cdcabrera Aug 25, 2017

cdcabrera Aug 25, 2017

cdcabrera Aug 25, 2017

cdcabrera Aug 25, 2017

cdcabrera Aug 25, 2017

cdcabrera Aug 25, 2017

beanh66 commented Aug 25, 2017

beanh66 left a comment

serenamarie125 left a comment

jeff-phillips-18 commented Aug 25, 2017

amarie401 commented Aug 25, 2017 •

edited

Loading

jeff-phillips-18 Aug 25, 2017

amarie401 Aug 25, 2017 •

edited

Loading

amarie401 Aug 28, 2017

jeff-phillips-18 Aug 28, 2017

jeff-phillips-18 commented Aug 28, 2017

amarie401 commented Aug 28, 2017

amarie401 commented Aug 28, 2017

cdcabrera left a comment

cdcabrera Aug 28, 2017

dtaylor113 left a comment

Alternate Label Placement for TrendsCard & TrendsChart #594

Alternate Label Placement for TrendsCard & TrendsChart #594

Conversation

amarie401 commented Aug 24, 2017 • edited Loading

Description

PR Checklist

amarie401 commented Aug 24, 2017 • edited Loading

jeff-phillips-18 commented Aug 24, 2017

beanh66 commented Aug 24, 2017

amarie401 commented Aug 24, 2017 • edited Loading

beanh66 commented Aug 24, 2017 • edited Loading

amarie401 commented Aug 24, 2017 • edited Loading

amarie401 commented Aug 25, 2017 • edited Loading

beanh66 commented Aug 25, 2017

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

beanh66 commented Aug 25, 2017

beanh66 left a comment

Choose a reason for hiding this comment

serenamarie125 left a comment

Choose a reason for hiding this comment

jeff-phillips-18 commented Aug 25, 2017

amarie401 commented Aug 25, 2017 • edited Loading

Choose a reason for hiding this comment

amarie401 Aug 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

jeff-phillips-18 commented Aug 28, 2017

amarie401 commented Aug 28, 2017

amarie401 commented Aug 28, 2017

cdcabrera left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dtaylor113 left a comment

Choose a reason for hiding this comment

amarie401 commented Aug 24, 2017 •

edited

Loading

amarie401 commented Aug 24, 2017 •

edited

Loading

amarie401 commented Aug 24, 2017 •

edited

Loading

beanh66 commented Aug 24, 2017 •

edited

Loading

amarie401 commented Aug 24, 2017 •

edited

Loading

amarie401 commented Aug 25, 2017 •

edited

Loading

amarie401 commented Aug 25, 2017 •

edited

Loading

amarie401 Aug 25, 2017 •

edited

Loading