Fixes Bug 775 - setting the chartGroup #834
Conversation
|
Change should be directed toward the |
|
Great, thanks! Yes there is a lot of annoying redundancy. I think the best we can do is make it consistent and deprecate where we can. Np about the branch. It's not too hard to redirect these, with care. When the branches diverge more it might get tricky. |
| expect(d3.select(chart.selectAll('g.dc-legend g.dc-legend-item')[0][3]).classed("fadeout")).toBeTruthy(); | ||
| expect(chart.selectAll("path.line").size()).toEqual(3); | ||
| expect(dateValueGroupLine2.classed("fadeout")).toBeFalsy(); | ||
| expect(chart.selectAll("path.line").size()).toEqual(1); |
There was a problem hiding this comment.
ugh, how are these related to the bug being fixed here? please tell me they are not.
There was a problem hiding this comment.
@mtraynham, so.... if you have to make seemingly random changes to the tests like this, that just might a sign that the changes broke something.
In this case, since the compositeChart generateG() calls child.chartGroup(_chart.chartGroup()), and the child charts have the same anchor as the parent, it ends up deregistering the composite chart and registering the child chart instead! So now all but one of the lines disappear when the chart rerenders.
I'll fix this up in another commit, once I figure out a good way. I guess this proves that testing is good, because it wasn't at all obvious that such a harmless-looking change could have such a disastrous effect. A little scary that this only broke one test, though.
There was a problem hiding this comment.
Good catch! I guess I was a little offset by the fact that only one test failed instead of all of them...
There was a problem hiding this comment.
the test is looking for a faded out legend item, and it also seems weird for there only to be one line remaining, so i was suspicious. i'm adding a flag for "this is a child chart", which seems like something we need to know anyway.
for #834 fixes #775 @mtraynham, hopefully this makes a bit more sense. i wonder if this is really the only place where we need to know whether a chart is a child chart
Fixes Bug #775
I think the logic between the two functions (i.e.
anchor&chartGroup) is correct now. I'm not really a fan of having more than one way of doing things (anchor,chartGroup,dc.chartRegistry), but I can't really pull this out for compatibility.Also, there was a weird test case from the composite mixin spec that inadvertently failed after my pretty minimal change. After some investigation, I think it was broken, but passing? Could use a second pair of eyes on that.