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

Dynamic Legend: Fix for Rickshaw GitHub Issue #89 #266

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

Conversation

mpderbec
Copy link
Contributor

This pull request fixes up the Legend class so that it can properly respond to series that are being dynamically added/removed. I modified the extensions.html example so that it (quickly) adds the series one at a time to the graph. This will hopefully expose any non-dynamic problems in the future.

Tests conducted:

  • Extensions.html thoroughly tested by manual operation.
  • Exercised Highlight, Toggle and Order behaviors.
  • Executed nodeunit tests.

- Re-factored Rickshaw.Graph.Legend considerably to allow for dynamic series additions/deletions.
- Removed somewhat brittle "series-reordering" code in Rickshaw.Behavior.Series.Highlight and added the concept of zOrder to the Renderer.
- Legend order had been reversed from previous version. Fixed.
- Rickshaw.Graph.Behavior.Series.Order.js interactions with the Legend were broken. Fixed.
… series at a time. This properly exercises the dynamic nature of the legend and the series behaviors.
@mpderbec
Copy link
Contributor Author

I will mention that I attempted to use pull request #201 prior to embarking on this set of changes… Unfortunately it did not fix the issue for all edge cases and all of the current extensions.

@dchester
Copy link
Contributor

dchester commented Aug 6, 2013

Thanks -- this is a good problem to solve. We have a branch around that aims to address some of these same issues. I'll take a pass through each and pull this together.

@rbu
Copy link
Contributor

rbu commented Apr 12, 2014

How is the dynamic series branch doing, btw? It probably needs some work being rebased to current master, is that worth a shot? @dchester, do you remember what issues you had with the work so far and what needs to be done to get that branch and/or this PR merged?

@rbu
Copy link
Contributor

rbu commented Apr 12, 2014

Probably should have that discussion on #341....

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

Successfully merging this pull request may close these issues.

4 participants