Switch to flotr2 library from flot for graphing #170

Closed
rgrp opened this Issue Jun 25, 2012 · 11 comments

2 participants

@rgrp
Open Knowledge International member
@lievenjanssen

I'm working on it. Got the graphs working, now changing the tooltips code.

@rgrp
Open Knowledge International member

@lievenjanssen that is amazing. How are you feeling that flotr2 stacks up against flot btw?

@lievenjanssen

Flotr2 introduces some extra functionality out of the box (e.g. tooltip)

By moving from jquery-flot to flotr2 there is no hard dependency on jquery anymore which is a good thing for such a large lib.

Unfortunately there was still a bug in the flotr2 tooltip code (mixed up east-west) but I fixed it and did a pull request on the flotr2 github. Can we temporarily add my fixed version in the vendor folder until the pull request gets merged in the flotr2 master branch?

@rgrp
Open Knowledge International member

@lievenjanssen I've just checked out the new flotr2 code in detail and it looks good. I have a few comments:

  • When putting stuff in the vendor folder can we specify the version (or failing that the date you got the code). E.g. we should have folder structure flotr2/{version-or-date-in-iso-format}/flotr2.js. I appreciate you have added your patched version so this is more complex :-) (i guess you'd do something like flotr2/2012-07-16/flotr2.js but with a note in the commit log.

  • Bar / column charts do not seem to use the name of the field if that field is a label rather than a number. To see this try out the demo

  • You hadn't patched the vendor requirements for the demos and tutorials (patch the _includes/recline-deps.html file) which meant they weren't working. A useful thing to note for any future patches. Also no update to the tutorial ... (this could wait on patching the dist). -- I've fixed this ...

  • If you use the date column you only seem to get the month

  • If you use columns or bars with dates you get JS number representation of the date rather than the date.

@lievenjanssen

couple of those issues were already in the original code as well. I'm doing a more thorough test of the graphs (my local vesion has some of those bugs fixed already) and will probably have a new pull request ready by the end of the weekend.

I will also incorporate all your comments.

@rgrp
Open Knowledge International member

amazing!

@lievenjanssen

During bug fixing I came across some functional considerations we have to decide on, these is the way I suggest to implement them:

Group Column (x-axis) is date:

  • show linear time axis for lines & points, lines and points graph types
  • show actual dates for bars and columns graph types (possibly sorted ascending by date)

Group Column (x-axis) is string:

  • group the data by that column and summarize the Value Columns (possibly sorted ascending by group column)

e.g.

Group Columnn, Value Column
UK , 10
DE , 5
UK , 5

Should become (grouped + sorted):
DE , 5
UK , 15

The aggregation function could also be chosen for the series, e.g. min, max, sum, avg, count.

@rgrp
Open Knowledge International member

@lievenjanssen - sorry for delay in responding (I could swear I submitted a comment on Monday but it doesn't seem to be here!)

I think this sounds good except for the last part on how we deal with repeated string values. In this case where x-axis (or y-axis for horizontal bar) is a string value I'd just use the index of the record to set its location (index will be 0,1,2,3 ...) and use the string value to set the label. This way we can cope with repetitions without any problem. This was the approach being used in the old bar charts (see https://github.com/okfn/recline/blob/a58f5e5bb06ed62cdf7d9f881fd8c389bad15b37/src/view.graph.js#L116 and https://github.com/okfn/recline/blob/a58f5e5bb06ed62cdf7d9f881fd8c389bad15b37/src/view.graph.js#L174)

@lievenjanssen

I know this was the case before but it seems odd to have repeated string values as you can't make sense out of them when displayed on a graph.

Of course grouping and aggregation could be handled by the backend as well.

I will leave the implementation of the repeated string values as it is (index to set location) and do a pull request later this week.

@rgrp
Open Knowledge International member

@lievenjanssen sounds great (while I agree re repeated values the primary use case will be where you don't have that -- e.g. you have a nice set of distinct values so all will work well!).

@rgrp rgrp added a commit that referenced this issue Sep 9, 2012
@rgrp rgrp [#170,cleanup][s]: remove jquery.flot vendor lib and last remaining r…
…ef to it in test/index.html as now replaced by flotr2 - fixes #170.
00d36f2
@rgrp rgrp added a commit that closed this issue Sep 9, 2012
@rgrp rgrp [#170,cleanup][s]: remove jquery.flot vendor lib and last remaining r…
…ef to it in test/index.html as now replaced by flotr2 - fixes #170.
00d36f2
@rgrp rgrp closed this in 00d36f2 Sep 9, 2012
@rgrp
Open Knowledge International member

FIXED. May be some minor tweaks remaining but conversion has been done and appears to be operating nicely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment