Skip to content

Conversation

@andykellr
Copy link

Issue 83

@andykellr
Copy link
Author

Sorry for the duplicate issue 83 and pull request 84.

@gwatts
Copy link
Owner

gwatts commented Jun 19, 2013

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
Copy link
Author

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
Copy link
Owner

gwatts commented Jun 19, 2013

Same option with function detection seems reasonable to me

…ort for function(sparkline,options,index,value)
@andykellr
Copy link
Author

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
Copy link
Author

Hey gwatts, any thoughts on this pull request?

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.

2 participants