Add donut chart pattern #200

jiekang · 2017-11-22T19:57:10Z

This adds the donut chart pattern addressing one half of issue #180

The patterns closely follow the angularJS versions:
http://www.patternfly.org/angular-patternfly/#/api/patternfly.charts.component:pfDonutChart

Notable differences:

similar to the patternfly-ng sparkline pattern, previously stand-alone options like chartHeight are moved under the config as for example config.chartHeight or config.centerLabel

A demo via rawgit is available via:
http://rawgit.com/jiekang/patternfly-ng/donut-chart-dist/dist-demo/#/donut

Thoughts?

joshuawilson · 2017-11-22T20:07:15Z

I think a lot of those commits can/should be squashed. A commit should be a change, if it takes you several commits to get it right but they are all the same thing then condense them. It keeps the change atomic.

jiekang · 2017-11-22T20:26:17Z

Okay. I've squashed the commits and following comments in #180 only included the donut chart pattern for this PR.

dlabrecq · 2017-11-22T20:28:09Z

Ideally, these components should be separate PRs -- it's a bit much to review at once.

If there are two components, they should have their own examples. Tabs can be used to show variations of that example.

Please include a link to a working example. A screenshot is also very helpful. I can download and build your code, but others (designers) will need to review.

There are instructions in the contribution guide for how to setup Travis to generate an example for you.

jiekang · 2017-11-22T21:28:47Z

I'd like to setup Travis to generate an example for me. The contribution guide states:

You will also need to add an AUTH_TOKEN variable to Travis generated in your GitHub account to allow Travis to connect to your fork.

Do you have more detailed instructions on how to accomplish this? My Travis build fails as it's not authorized to push to my repository.

In the meantime I've attached a link to a screenshot of the donut example page:

Screenshot

sahil143 · 2017-11-23T08:30:07Z

@jiekang
Travis Build
You can follow this step by step guide to add AUTH_TOKEN to Travis. Remember, you need to do this for Patternfly-ng instead of Patternfly-webcomponents.

jiekang · 2017-11-23T13:53:28Z

@sahil143 Thank you!

jiekang · 2017-11-23T13:54:30Z

A demo via rawgit is available at:

http://rawgit.com/jiekang/patternfly-ng/donut-chart-dist/dist-demo/#/

The donut chart is routed at:

http://rawgit.com/jiekang/patternfly-ng/donut-chart-dist/dist-demo/#/donut

dlabrecq · 2017-11-24T01:19:32Z

Does the donut chart provide the ability to add horizontal and vertical labels like the Angular PatternFly example?

jiekang · 2017-11-24T15:42:44Z

@dlabrecq
I don't see any mention of horizontal or vertical labels in the API [1] or source [2] for donut charts.
[1] http://www.patternfly.org/angular-patternfly/#/api/patternfly.charts.component:pfDonutChart
[2] https://github.com/patternfly/angular-patternfly/blob/master/src/charts/donut/donut-chart-component.js

For context, the code is as close to a 1 to 1 transform of js to ts and angularjs to angular that tries to follow the style of the sparkline chart that's already in patternfly-ng. The only major differences are:

Does not have dataAvailable html guard to switch display to pf-empty-chart. I don't think this element exists yet in patternfly-ng
Uses the EventEmitter provided by ChartBase to access chart when it's available. The angularjs version accesses chart directly.
Merges defaults on config and config.donut as the defaults function is not recursive and donut contains default configuration that we'd like to keep. Otherwise a user setting the title via donut.title will lose all default configuration in config.donut. In angularjs it seems the merge is recursive as this problem isn't seen with the exact same example.

In general, there should be no functional difference between the angular and angularjs versions of the donut chart. At least that's what I intended.

I see a labelConfig in the donut-pct API [3]. Is that what you are referring to? It is not available in the donut chart.
[3] http://www.patternfly.org/angular-patternfly/#/api/patternfly.charts.component:pfDonutPctChart

dlabrecq · 2017-11-25T05:06:03Z

The labels I'm referring to in the Angular PatternFly donut chart example below are; Cats, Hamsters, Fish, and Dogs.

jiekang · 2017-11-27T13:53:08Z

Ah, yes users can add a legend. In general, users can add c3 configuration in the same manner as before (which is how the legend is added in the angularjs example).

@dlabrecq Would you like me to modify the angular example to include exactly the two that you showed?

dlabrecq · 2017-11-27T14:15:12Z

Yes, please do. Need to ensure the examples can demonstrate feature parity with APF.

jiekang · 2017-11-27T15:20:25Z

@dlabrecq Okay, done.

Looks like the rawgit has automatically updated and the result can be viewed there.

dlabrecq

Please update the example to show tooltips like Angular PatternFly.

dlabrecq · 2017-11-27T16:10:09Z

src/app/chart/donut/donut-config.ts

+  /**
+   * An optional function to customize the text of the center label
+   */
+  centerLabelFn?: any;


In general, we want to stay away from callback functions. Instead, this can be a simple centerLabel property.

Changed to centerLabel property.

dlabrecq · 2017-11-27T16:11:32Z

src/app/chart/donut/donut-config.ts

+  /**
+   * An optional function to handle when donut is clicked
+   */
+  onClickFn?: any;


Instead of a callback function, we would like to have an onClick event in the component.

This onClickFn mirrors angular-patternfly and would be an API difference between angular-patternfly and patternfly-ng if changed, is that okay? My approach was to keep things one to one such that anyone deciding to port from one to the other would not have to make any config changes due to incompatibilities. However I realize angularjs and angular are completely different projects so this might not be a concern for this project.

I don't understand what an "onClick event in the component" means, could you elaborate?

Yes, it's ok to be different from APF. This is our opportunity to clean things up.

Instead of calling a function when the donut is clicked, we emit an event. A developer can easily subscript to the event like so:

<pfng-donut-chart (onClick)="myFunction($event)"...

Then, in the component we would have something like this:

/**
* The event emitted when an item has been clicked
*/
@output('onClick') onClick = new EventEmitter();

And some method to handle the click:

handleClick($event: MouseEvent): void {
  this.onClick.emit({
    item: item
  } as ListEvent);
}

dlabrecq · 2017-11-27T16:14:51Z

src/app/chart/donut/donut-config.ts

+  /**
+   * C3 inherited configuration for colors
+   */
+  colors?: any;


It's not clear what "colors" is. Please add an example to the comments? For example:

'colors' : {
  'Cats': '#0088ce', // blue
  'Hamsters': '#3f9c35', // green
  'Fish': '#ec7a08', // orange
  'Dogs': '#cc0000' // red
}

I wonder if a typed object may help here?

colors?: Color;

I've updated the comment with an example. I'm reluctant to add a typed object for c3 inherited configuration as it mimics c3 configuration exactly. Maybe a link to the c3 API that it's intended to match would work for all c3 inherited configuration?

dlabrecq · 2017-11-27T16:17:45Z

src/app/chart/donut/donut.component.html

@@ -0,0 +1 @@
+<div #chartElement id="{{donutChartId}}"></div>


We will need to create the pf-empty-chart for the no data available use case. This is an integral part of the component HTML layout in Angular Patternfly. It should be a very simple component -- please let me know if you need help with that?

I presume this is another issue and PR? I can take a look at porting this. I suppose it's:

https://github.com/patternfly/angular-patternfly/blob/master/src/charts/empty-chart.component.js
https://github.com/patternfly/angular-patternfly/blob/master/src/charts/empty-chart.html

dlabrecq · 2017-11-27T16:22:32Z

src/app/chart/donut/donut.component.spec.ts

+  it('should setup onclick correctly', () => {
+    expect(typeof(comp.config.data.onclick)).toBe('function');
+  });
+});


These tests are a great start.

In addition to these tests, I would like to see a couple tests ported from donut-pct.spec.js. For example, anything that tests the label would be very useful here. The donut-pct tests seem to share a lot in common with donut.

dlabrecq · 2017-11-27T16:59:51Z

src/demo/app-routing.module.ts

@@ -20,6 +20,7 @@ import { SearchHighlightExampleComponent }
 import { SortExampleComponent } from '../app/sort/examples/sort-example.component';
 import { SortArrayExampleComponent } from '../app/pipe/sort-array/examples/sort-array-example.component';
 import { SparklineExampleComponent } from '../app/chart/sparkline/examples/sparkline-example.component';
+import { DonutExampleComponent } from '../app/chart/donut/examples/donut-example.component';


Alphabetical order please

Done.

Thoughts on adding ordered-imports and the like to enforce tslinting of alphabetical order as a separate issue/PR? Also I noticed tslint was updated from 4.x to 5.x and the tslint config file contains unsupported configuration for member-ordering. It doesn't seem to break anything but I think a fix would be good for those using editors that read from the config file to show tslint warnings.

Yes, it would be good to fix this. A separate PR would probably be best.

The member-ordering seems to be working; for example, if I add a private method before a public one. Although, it's possible that config could be deprecated?

Unfortunately, none of that enforces import ordering, just fields, methods, and constructors. However, I did find adding this does the trick:

"ordered-imports": true,

dlabrecq · 2017-11-27T17:00:06Z

src/demo/app-routing.module.ts

@@ -85,6 +86,9 @@ const routes: Routes = [{
  }, {
    path: 'sparkline',
    component: SparklineExampleComponent
+  }, {
+    path: 'donut',


Alphabetical order please

dlabrecq · 2017-11-27T17:00:14Z

src/demo/app.component.html

@@ -37,6 +37,9 @@ <h3>Components</h3>
            <li>
              <a routerLink="/sparkline" routerLinkActive="active">Sparkline</a>
            </li>
+            <li>
+              <a routerLink="/donut" routerLinkActive="active">Donut</a>


Alphabetical order please

dlabrecq · 2017-11-27T17:00:23Z

src/demo/app.module.ts

@@ -55,6 +56,7 @@ import { NavigationExampleModule } from '../app/navigation/examples/navigation-e
    SortArrayExampleModule,
    SortExampleModule,
    SparklineExampleModule,
+    DonutExampleModule,


Alphabetical order please

dlabrecq · 2017-11-27T17:00:27Z

src/demo/app.module.ts

@@ -27,6 +27,7 @@ import { SearchHighlightExampleModule } from '../app/pipe/search-highlight/examp
 import { SortArrayExampleModule } from '../app/pipe/sort-array/examples/sort-array-example.module';
 import { SortExampleModule } from '../app/sort/examples/sort-example.module';
 import { SparklineExampleModule } from '../app/chart/sparkline/examples/sparkline-example.module';
+import { DonutExampleModule } from '../app/chart/donut/examples/donut-example.module';


Alphabetical order please

…bscription

… and config updates

…t config comments

dlabrecq · 2017-11-28T23:19:03Z

src/app/chart/donut/examples/donut-example.component.ts

+    Observable
+    .timer(0, 5000)
+    .map(() => Math.floor(Math.random() * 100) + 100)
+    .subscribe(val => this.config2.chartHeight = val);


This is very cool, but can we add this as a separate example? For example, have a "Basic" and "Dynamic" tabs similar to other component examples.

Sometimes my tooltip disappears as I'm attempting to hover and the data changes under me.

As an example, we could have examples named donut-basic-example.component.ts and donut-dynamic-example.component.ts...

dlabrecq

LGTM

jiekang changed the title ~~Add donut and donut-pct chart patterns~~ Add donut chart pattern Nov 22, 2017

feat(donut): add donut chart pattern

83db8e8

refactor(donut): update donut chart example

6f759d8

dlabrecq requested changes Nov 27, 2017

View reviewed changes

jiekang added 5 commits November 27, 2017 14:38

chore(donut): fix order of imports and declarations to be alphabetical

8d09d21

refactor(donut): fix update implementation and manage chart loaded su…

7f9fe68

…bscription

temp(donut): temporarily refactor example to demonstrate dynamic data…

e950066

… and config updates

fix(donut): use center label text instead of function and update donu…

7711c39

…t config comments

fix(donut): add donut specific tooltip

d33d2a9

dlabrecq reviewed Nov 28, 2017

View reviewed changes

jiekang added 2 commits November 30, 2017 09:12

refactor(donut): add more tests for donut chart

7fee6a8

refactor(donut): use unique chart id built from config.chartId

6286731

dlabrecq approved these changes Nov 30, 2017

View reviewed changes

dlabrecq merged commit be620bf into patternfly:master Nov 30, 2017

		@@ -0,0 +1 @@
		<div #chartElement id="{{donutChartId}}"></div>

Add donut chart pattern #200

Add donut chart pattern #200

Conversation

jiekang commented Nov 22, 2017 • edited

joshuawilson commented Nov 22, 2017

jiekang commented Nov 22, 2017 • edited

dlabrecq commented Nov 22, 2017 • edited

jiekang commented Nov 22, 2017 • edited

sahil143 commented Nov 23, 2017

jiekang commented Nov 23, 2017

jiekang commented Nov 23, 2017

dlabrecq commented Nov 24, 2017

jiekang commented Nov 24, 2017

dlabrecq commented Nov 25, 2017

jiekang commented Nov 27, 2017

dlabrecq commented Nov 27, 2017

jiekang commented Nov 27, 2017

dlabrecq left a 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

dlabrecq Nov 28, 2017 • edited

Choose a reason for hiding this comment

dlabrecq Nov 27, 2017 • edited

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

jiekang Nov 27, 2017 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlabrecq left a comment

Choose a reason for hiding this comment

jiekang commented Nov 22, 2017 •

edited

jiekang commented Nov 22, 2017 •

edited

dlabrecq commented Nov 22, 2017 •

edited

jiekang commented Nov 22, 2017 •

edited

dlabrecq Nov 28, 2017 •

edited

dlabrecq Nov 27, 2017 •

edited

jiekang Nov 27, 2017 •

edited