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

Bubble Chart: selectAll should be select when manipulating a enter/update selection #1237

Closed
wants to merge 1 commit into from

Conversation

mtraynham
Copy link
Contributor

selectAll in these cases is going to screw up the current enter/update selection, which results in some nodes not getting updated. These should really be select. I feel like this has come up before, but I can't seem to find the particular PR or conversation about it.

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Dec 14, 2016

Yes, #1032 is the previous issue. And you found similar problems in four other charts. So I guess we are not testing updates of at least these six charts. 😱

@gordonwoodhull gordonwoodhull added this to the v2.0 milestone Dec 14, 2016
@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Dec 14, 2016

It's not that we don't test update. It's that the code currently works as long as the data is updated in-place, as crossfilter always does.

But we definitely shouldn't assume that.

On a hunch, I created a (rather excessive) bl.ock demonstrating the problem:
http://bl.ocks.org/gordonwoodhull/14c623b95993808d69620563508edba6

Everything is fine with the top two charts, which use straight crossfilter. But in the second two charts we clone the keys and values each time group.all() is called. Wham, no updates get through to those charts, because they still have the original objects and don't see the changed reductions.

gordonwoodhull added a commit that referenced this pull request Dec 18, 2016
for #1237
also, new iris test fixture
@gordonwoodhull
Copy link
Contributor

added test and merging for 2.0 - thanks @mtraynham!

gordonwoodhull added a commit that referenced this pull request Dec 19, 2016
for #1237
this tests the bubble mixin
gordonwoodhull added a commit that referenced this pull request Dec 19, 2016
honest, it's not just to puff up my test count
it's to make it clear these are all separate functions being tested
for #1237
@mtraynham mtraynham deleted the bubble_selectAll_select branch January 5, 2017 13:52
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