Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New Donut chart and Pie chart #149

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

tiraeth
Copy link
Contributor

@tiraeth tiraeth commented Dec 31, 2012

As in my previous PR, new version for these two folks. I've also added callbacks for hover and click events.

Currently only one series per donut chart but in future more series will be available to be put there. These charts support 0-valued rows by providing @options.minSectorAngle (in degrees) to be used as a minimum value for a sector.

Please mind that if minSectorAngle: 5 then sectors with values 0 and 3 will have the same angle in the chart.

  • Labels are displayed above the chart (@options.showLabel = (true|false|"hover")),
  • You can define the draw-out radius using @options.drawOut (defaults to 5) which will affect outer (and inner in case of Donut) arc,
  • You can define the sector ID key (@options.idKey) to identify clicked/hovered sectors in callbacks (you also get the whole data row extended with sector %-value and angle degree-value),
  • Single row data is handled properly in each chart type,
  • Uniform data set with zeros is also handled properly (i.e. 4 rows with zeros will get 4 equal quarters on chart).

donut-chart pie-chart

Hint: I am very seriously thinking about getting rid of the stroke parameter and handle the spacing between sectors with math instead of path's stroke.

PS. Sorry for two commits per PR. Forgot to rebase 😢

@gbougeard
Copy link

Does it allow to add click handler on sectors? If it is, how do we do?

@tiraeth
Copy link
Contributor Author

tiraeth commented Jan 22, 2013

Yes it does.

var options = {}; // you know what to do with it...
var chart = new Morris.Pie(options);

chart.on('click', function(id, row){
  console.log(id, row);
});

chart.on('hover', function(id, row){
  console.log(id, row);
});

@caarlos0
Copy link

👍 to this!

@merencia
Copy link

👍

@EdwardDiehl
Copy link

Tried adding click handler on sectors. It works well. Hope it will be merged with master.

@richardjortega
Copy link

+1 to this too!

@tsileo
Copy link

tsileo commented Feb 23, 2013

👍

2 similar comments
@todddickerson
Copy link

+1

@abirmingham
Copy link

+1

@ebiasini
Copy link

+1

@kazzkiq
Copy link

kazzkiq commented Mar 22, 2013

@tiraeth:

Hint: I am very seriously thinking about getting rid of the stroke parameter and handle the spacing between sectors with math instead of path's stroke.

I agree with this approach, by now the only ways to change stroke color on pie/donut charts that i know are by editing the plugin's source or changing it via Raphael.js inline (i've never tried that last).

Maybe a good idea is to set a parameter like strokeColor accepting RGBA, which can easily solve this problem without doing mess with who like this white stroke spacing.

@benyitzhaki
Copy link

hi, why haven't you merged this with the master? any issues? is it ready for production?

@oesmith
Copy link
Contributor

oesmith commented Apr 9, 2013

Two reasons:

  1. Since joining Google I've had a lot less spare time to spend on Morris.js. Big PRs are more time-consuming to review and merge, especially where there's a number of dependent new features being added.
  2. I don't think moving the label outside of the donut is a good direction to move in. It certainly doesn't fit the use case I have for donuts.

@caarlos0
Copy link

caarlos0 commented Apr 9, 2013

I completely understand you, @oesmith.

Maybe is time for you to elect one or more fellows to help you maintain the
project (it's a suggestion).

On Tue, Apr 9, 2013 at 5:26 PM, Olly Smith notifications@github.com wrote:

Two reasons:

  1. Since joining Google I've had a lot less spare time to spend on
    Morris.js. Big PRs are more time-consuming to review and merge, especially
    where there's a number of dependent new features being added.
  2. I don't think moving the label outside of the donut is a good
    direction to move in. It certainly doesn't fit the use case I have for
    donuts.


Reply to this email directly or view it on GitHubhttps://github.com/oesmith/morris.js/pull/149#issuecomment-16137859
.

Atenciosamente,
Carlos Alexandro Becker
http://caarlos0.github.com/about

@tiraeth
Copy link
Contributor Author

tiraeth commented Apr 9, 2013

I do understand your POV. But I think that if you want Morris to be used wider (and I think you want) some functionality should be changed to use the ones users are used to.

I think that tooltips should be controlled by CSS (html ones, like in other charts). Displaying the value (and label) as you do right now could be controlled by hover event on the chart. I think that you should support the conventional behaviour and give people a way to accomplish more sophisticated changes.

And more, I think that donut chart should in near future support multiple series, because that's the purpose of displaying data in donut instead of a pie.

@oesmith
Copy link
Contributor

oesmith commented Apr 9, 2013

My philosophy for Morris.js is for it to be a simple charting library. It's the one you reach for when you want to do something quick and simple, without sacrificing aesthetics.

Features like milti-dimensional donut charts and rich interactivity are beyond that philosophy. I'd very much prefer people to be using more advanced visualisation libraries like D3.

@tiraeth
Copy link
Contributor Author

tiraeth commented Apr 9, 2013

Usage simplicity (and small amount of code) does not imply the lack of some functionaltiy. But I understand you and won't insist on pushing functionality you object to. Others can always use one of forked repositories (mine, for instance) if they want to.

Thanks for the great work you are doing here 👍

@sennor
Copy link

sennor commented Apr 26, 2013

@tiraeth - How to create a pie chart with your forked repository of morris.js? Can´t find any examples..

@tiraeth
Copy link
Contributor Author

tiraeth commented Apr 26, 2013

See /examples directory. These are using the new charts.

@AlexandreArpin
Copy link

+1

The hover and click binding would allow for a wide range of customisation, without any added complexity to the current usage.

@blanchma
Copy link

Is this merged on master already?

@danpe
Copy link

danpe commented Dec 11, 2013

👍

1 similar comment
@gfazioli
Copy link

👍

@sirNemanjapro
Copy link

Anything happening?

if @el is null or @el.length == 0
throw new Error("Container element not found.")

if options.data is undefined or options.data.length is 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't great since if we're going to create Pie chart with initial options and only then set data using setData() & call redraw() we will get Exception while trying to access @options object since it wasn't initialized in constructor. I do realise the same code was before that in Donut class.

@sudodoki
Copy link
Contributor

Coming back to this PR have few questions:

  1. @oesmith , are you objecting including this Pie-Componennt into core library at all?
  2. @tiraeth, since Morris is rather straightforward, why not implement this as a plugin/component? I did it like this for my build and with few patches it works like a charm.
    On this matter, too: we should have just a set of common methods, like setData/render etc (this should be laid out somewhere) and anybody will be able to contribute his/her plugin for Morris. I would love to be able to use heatmaps, if needed, without need to switch to some other library.

@sudodoki sudodoki mentioned this pull request Jan 30, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.