search

gwatts / jquery.sparkline

A plugin for the jQuery javascript library to generate small sparkline charts directly in the browser
http://omnipotent.net/jquery.sparkline/
1.24k stars 278 forks source link

Implementation of colorMap as a function(sparkline,options,index,value) #84

Open andykellr opened 11 years ago

andykellr commented 11 years ago

Issue 83

andykellr commented 11 years ago

Sorry for the duplicate issue 83 and pull request 84.

gwatts commented 11 years ago

This is a good idea.. the callback should probably more closely mirror the tooltipFormatter arguments which also passes in the reference to the sparkline object and options.

It should also apply to the tristate chart type and at that point, the whole shebang could probably be refactored a bit so that there's less code duplication :-/

andykellr commented 11 years ago

I'll look at the tooltipFormatter. A ref to the sparkline makes sense. If I weren't trying to match another callback, I would probably skip options since they appear to be accessible as .options.

I agree about adding it to tristate as well and about refactoring it. I did as much as I needed without knowing if you were interested in this approach. I'm happy to spend a little more time.

Do you think using colorMap in this way makes sense? I also considered adding a separate option named colorFunction, but ultimately decided that since they couldn't be used together (one would take precedence), they should use the same option.

gwatts commented 11 years ago

Same option with function detection seems reasonable to me

andykellr commented 11 years ago

Update refactors colorMap and gets rid of the colorMapByIndex/Value/Function variables and uses a single colorMapFunction instead. initColorMap() in base.js will create the appropriate functions based on whether an array, function, or other value is supplied.

While I was in there, I changed the tristate calcColor to use value instead of values[valuenum] since they are the same.

Let me know if you'd like any other changes.

andykellr commented 11 years ago

Hey gwatts, any thoughts on this pull request?