Utilization Donut Chart Component #570

cdcabrera · 2017-08-12T06:06:01Z

Description

Implementation for utilization donut chart component.

Create a CPU Utilization Component, which is not embedded in a Card:

color changes based on thresholds
optional details are shown
optional details can be stacked horizontally, after the chart
optional details can be stacked vertically

In Progress Checklist

Unit tests
CSS styling for labels
Clean up and line endings
Evaluate focusing on use of a C3 chart config instead of individual attributes, similar to existing chart components
Evaluate transcluded label instead of "parameterizing" and "concatenating" them

PR Checklist

Unit tests are included
Screenshots are attached (if there are visual changes in the UI)
A Designer is assigned as a reviewer (if there are visual changes in the UI)
A CSS rep is assigned as a reviewer (if there are visual changes in the UI)
Amend commit for continuous integration

Updated

Sizing and placement were/are the more bothersome properties to deal with and one of the reasons for separating it out. Now that understand how sizing functions I can look at what converting this version would take, since it sounds like we're good with the add on approach. This works as part of the conversion from attributes back to the config as part of the checklist above

cdcabrera · 2017-08-14T14:02:38Z

@dtaylor113 have part of your input, @jeff-phillips-18 do you have input on...

Adding to the existing pct-donunt chart component?

And both...

The idea that a large percentage of this component, as is, is just structuring the "config" in a customized way, and that perhaps just placing an updated demo might be enough?
Here's a quick snapshot of the intention behind it...

serenamarie125 · 2017-08-14T14:13:20Z

@cdcabrera agree with the approach of updating the existing donut chart rather than create a new component

cdcabrera · 2017-08-14T14:17:56Z

good good @serenamarie125 , @beanh66 @cshinn

aspects of the initial design layout can start to be viewed, used flex to layout the columns. There's an initial screen shot, which when compared to the request is missing some bold and sized text

@beanh66 @cshinn your input on

adding to the existing component vs writing a new one, that's really really close to the old one.

dtaylor113 · 2017-08-14T14:21:11Z

@cdcabrera, you are right that we already have an attribute called 'center-label' which can have values 'used', 'available', 'percent', and 'none', so it sounds like setting it to 'percent' will correctly set the center label. The issue is how can we bundle the 'side labels' so that it's easy for app. dev to customize/specify.
I would suggest adding a config option, something like 'side-label': {
position: 'left' / 'right',
title: 'Storage',
type: percent, used, available (?),
format: full ('of m GB used'), idk, just spitballing here :-)
}

jeff-phillips-18 · 2017-08-14T15:35:53Z

@cdcabrera I agree with enhancing the existing component.

cdcabrera · 2017-08-14T18:43:10Z

@amarie401 for reference

jeff-phillips-18 · 2017-08-15T12:47:52Z

Is this still WIP or ready for final reviews?

cdcabrera · 2017-08-15T13:01:32Z

@jeff-phillips-18 et all, it's close to final can remove the WIP just...

need some for/against input on how the additional config option is structured on top of the existing donut
any minor design concerns

Then i'll rebase and amend the commit message

jeff-phillips-18

I'm OK with the additional config option.

jeff-phillips-18 · 2017-08-18T14:03:40Z

src/charts/charts.less

+    flex-direction: row-reverse;
+  }
+}
+


Should this really be put into Patternfly? Is this a Patternfly pattern?

@jeff-phillips-18 can move it over to core for sure np. There are aspects of charts both within this repository's less and core, just need a direction we want to head?

After looking a little bit more on this, looks like we have a choice, move now, or move later as a whole. The appearance is, it looks like we attempted to start a PF core aspect to the charts, here:

https://github.com/cdcabrera/patternfly/blob/master/src/less/charts.less

https://rawgit.com/patternfly/patternfly/master-dist/dist/tests/donut-charts.html

With a distinction about only how we modify SVG aspects of the Charts within AngularPF. However it looks like we slightly diverged a little and started placing a few more bits inside AngularPF instead of core.

I can go back and move this newer piece over to core now, or hold off based on what's already in AngularPf? But in addition, outside of this PR, we probably need to go through the rest of charts.less and separate them out a bit more.

@jeff-phillips-18 @dtaylor113 the less has been moved over to core Patternfly PR 720

jeff-phillips-18 · 2017-08-18T14:05:15Z

src/charts/donut/examples/donut-pct-chart.js

@@ -26,6 +26,15 @@
 * <li>.tooltipFn(d)   - user defined function to customize the tool tip (optional)
 * <li>.centerLabelFn  - user defined function to customize the text of the center label (optional)
 * <li>.onClickFn(d,i) - user defined function to handle when donut arc is clicked upon.
+ * <li>.label          - object containing properties for external label (optional)
+ *   <ul>
+ *       <li>.orientation - string with possible values: 'left', 'right' (optional)


This is optional, what is the default?

Good catch can update the comments/annotation. For our purposes the optional default is it simply doesn't exist, don't have to use it

jeff-phillips-18 · 2017-08-18T14:07:03Z

src/charts/donut/examples/donut-pct-chart.js

+ *   <ul>
+ *       <li>.orientation - string with possible values: 'left', 'right' (optional)
+ *       <li>.title       - string representing a prefix or title (optional)
+ *       <li>.label       - similar in to the center-label parameter, which specifies the contents of the donut's external label, values: 'used', 'available', 'percent', 'none' (optional)


I don't understand the description here.

Yeah the wording is a little funky reading back on it, can correct it...

jeff-phillips-18 · 2017-08-18T14:08:21Z

src/charts/donut/examples/donut-pct-chart.js

+               <pf-donut-pct-chart config="configDynamic" data="dataDynamic" center-label="labelDynamic"
+                                   on-threshold-change="thresholdChanged(threshold)">
+               </pf-donut-pct-chart>
+             </p>
           </div>
           <div class="col-md-3 text-center"">


Not this PR but coul you remove the extra " character.

jeff-phillips-18 · 2017-08-18T14:12:04Z

src/charts/donut/examples/donut-pct-chart.js

@@ -26,6 +26,15 @@
 * <li>.tooltipFn(d)   - user defined function to customize the tool tip (optional)
 * <li>.centerLabelFn  - user defined function to customize the text of the center label (optional)
 * <li>.onClickFn(d,i) - user defined function to handle when donut arc is clicked upon.
+ * <li>.label          - object containing properties for external label (optional)


Maybe this should be labelConfig or something more descriptive so that it is understood that this is not just a simple label.

I had suggested sideLabel

Hi, I don't think .label should be under config, rather it should be a separate param like center-label is, and called side-label with properties orientation, title, and perhaps something like 'amount-type' for available, percent, used.

Took using side-label into consideration, but when the label can be used on the right, left, and bottom it becomes a slight misnomer, so changed it to label.

Was also going to create it as a separate attribute similar to center-label, but then we blend the use of how center-label functions. We have both a configuration aspect, which takes a function, and the attribute, which takes a string. Based on that took the configuration route all in one, instead of a blended approach, which for myself felt less confusing.

I do agree the label label should probably be changed to something else, I like labelConfig, had also thought maybe outsideLabel, but the Config suffix seems more appropriate.

As a quick fix, I can go ahead and change the label to labelConfig while we continue to discuss.

dtaylor113 · 2017-08-21T13:03:02Z

src/charts/donut/donut-pct-chart.html

+    </span>
+    <span ng-if="$ctrl.data.dataAvailable !== false && $ctrl.config.label && !$ctrl.config.label.labelFn()" class="pct-donut-chart-pf-label">
+      {{$ctrl.config.label.title}}
+      <span ng-if="$ctrl.data" ng-switch="$ctrl.config.label.label">


$ctrl.config.label.label I think we do need 'labelConfig' or something clearer. Currently, 'label.label' is tough to read.

dtaylor113

LGTM, aside from where the css/less goes

cdcabrera · 2017-08-22T18:04:12Z

@dtaylor113 @jeff-phillips-18 since both commented on the Less, I'll go ahead and look at moving ~~move~~ it over to PF core for this update. If there are any additional PCT Donut styles that don't fall into the SVG/C3 manipulate I can try to move those as well.

EDIT: I don't necessarily agree with moving this to core by itself, unless we move the entirety of the charts.less

cdcabrera · 2017-08-25T14:05:32Z

Went ahead and separated out the Pct Donut Less towards core Pf, see this PR 720

jeff-phillips-18 · 2017-08-28T14:32:36Z

src/charts/charts.less

@@ -11,6 +11,7 @@ pf-c3-chart {
    width: 100%;
  }
 }
+


Looks like this file's changes could be discarded. No point updating to include an empty line 😉

jeff-phillips-18 · 2017-08-28T14:38:01Z

src/charts/donut/examples/donut-pct-chart.js

+ *       <li>.title       - string representing a prefix or title (optional) - default: empty string
+ *       <li>.label       - the wording format to display, possible values: 'used', 'available', 'percent', 'none' (optional) - default: 'used'
+ *       <li>.units       - unit label for values, ex: 'MHz','GB', etc.. (optional) - default: empty string
+ *       <li>.labelFn     - function to customize the text of the external label (optional) - default: undefined


Should this function take some parameters? How is the function to know what the label ought to be?

Can add a note/annotation on there.

The behavior is similar to the existing centerLabelFn callback. That one also seems to be lacking in the documentation/annotation department

dtaylor113 · 2017-08-28T14:45:23Z

Hi @cdcabrera, I feel like the ngDoc has somewhat gotten somewhat munged (if that is a word :-). Previously, the top row was showing the different Thresholds, the middle row was the different center-labels, the last row was some custom stuff:

With your current changes, the first row is now showing Thresholds and some custom label placements, and I think different donut sizes(?). However, there is no ngDoc help text or labels to denote what is being displayed in the first row:

I would suggest leaving the first row to denote the different Thresholds and add a new row which clearly describes the different/new label customizations and placements you are introducing.
Thanks,
Dave

Wont change to 'Changes requested'

dtaylor113

Please update ngDoc examples

cdcabrera · 2017-08-28T15:07:49Z

@dtaylor113 can add a dedicated row for sure

cdcabrera · 2017-08-28T15:09:46Z

This PR is dependent on core Patternfly PR 720 for CSS

cdcabrera · 2017-08-28T21:06:59Z

@dtaylor113 the revised layout linked to Patternfly core PR 720

Edit: included the wrong image... corrected below...

Enhancement for utilization donut chart component

cdcabrera · 2017-08-28T23:57:01Z

The associated core PatternFly PR 720 has been merged, you'll need to update your packages to see the CSS update

dtaylor113 · 2017-08-29T12:38:44Z

@cdcabrera Perfect! Exactly what I was envisioning.

cdcabrera changed the title ~~[WIP][NO-REVIEW] Utilization Donut Chart Component~~ [WIP] Utilization Donut Chart Component Aug 14, 2017

cdcabrera force-pushed the feature-utilization branch from 58fe455 to 7d25833 Compare August 14, 2017 20:17

cdcabrera requested review from cshinn, dtaylor113, beanh66 and jeff-phillips-18 August 14, 2017 20:22

cdcabrera changed the title ~~[WIP] Utilization Donut Chart Component~~ Utilization Donut Chart Component Aug 15, 2017

cdcabrera force-pushed the feature-utilization branch from 7d25833 to ed38af3 Compare August 16, 2017 18:35

jeff-phillips-18 requested changes Aug 18, 2017

View reviewed changes

dtaylor113 reviewed Aug 21, 2017

View reviewed changes

cdcabrera force-pushed the feature-utilization branch from ed38af3 to 02f0e4e Compare August 22, 2017 14:04

dtaylor113 previously approved these changes Aug 22, 2017

View reviewed changes

This was referenced Aug 22, 2017

Angular PatternFly Charts Less Not in PatternFly patternfly/patternfly-3#718

Closed

feat(chart): Utilization Donut Chart Component patternfly/patternfly-3#720

Merged

cdcabrera force-pushed the feature-utilization branch from 65dbe68 to b92fe01 Compare August 23, 2017 18:59

jeff-phillips-18 reviewed Aug 28, 2017

View reviewed changes

jeff-phillips-18 requested changes Aug 28, 2017

View reviewed changes

dtaylor113 suggested changes Aug 28, 2017

View reviewed changes

cdcabrera force-pushed the feature-utilization branch from b92fe01 to 567bbe7 Compare August 28, 2017 21:05

cdcabrera force-pushed the feature-utilization branch from 567bbe7 to c514361 Compare August 28, 2017 21:16

feat(chart): Utilization Donut Chart Component

cf4c16c

Enhancement for utilization donut chart component

cdcabrera force-pushed the feature-utilization branch from c514361 to cf4c16c Compare August 28, 2017 21:21

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

View reviewed changes

dtaylor113 merged commit 469fe4d into patternfly:master Aug 29, 2017

Utilization Donut Chart Component #570

Utilization Donut Chart Component #570

Uh oh!

Conversation

cdcabrera commented Aug 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

In Progress Checklist

PR Checklist

Updated

Previous

Uh oh!

dtaylor113 commented Aug 14, 2017

Uh oh!

cdcabrera commented Aug 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cdcabrera commented Aug 14, 2017

Uh oh!

serenamarie125 commented Aug 14, 2017

Uh oh!

cdcabrera commented Aug 14, 2017

Uh oh!

dtaylor113 commented Aug 14, 2017

Uh oh!

jeff-phillips-18 commented Aug 14, 2017

Uh oh!

cdcabrera commented Aug 14, 2017

Uh oh!

jeff-phillips-18 commented Aug 15, 2017

Uh oh!

cdcabrera commented Aug 15, 2017

Uh oh!

jeff-phillips-18 left a comment

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!

cdcabrera Aug 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdcabrera Aug 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdcabrera Aug 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

dtaylor113 Aug 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dtaylor113 Aug 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdcabrera Aug 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dtaylor113 left a comment

Choose a reason for hiding this comment

Uh oh!

cdcabrera commented Aug 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

cdcabrera commented Aug 12, 2017 •

edited

Loading

cdcabrera commented Aug 14, 2017 •

edited

Loading

cdcabrera Aug 22, 2017 •

edited

Loading

cdcabrera Aug 23, 2017 •

edited

Loading

cdcabrera Aug 18, 2017 •

edited

Loading

dtaylor113 Aug 18, 2017 •

edited

Loading

dtaylor113 Aug 18, 2017 •

edited

Loading

cdcabrera Aug 18, 2017 •

edited

Loading

cdcabrera commented Aug 22, 2017 •

edited

Loading

cdcabrera commented Aug 28, 2017 •

edited

Loading

cdcabrera commented Aug 28, 2017 •

edited

Loading