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

Append choropleth titles by default #1080

Closed
wants to merge 1 commit into from

Conversation

ferblape
Copy link

@ferblape ferblape commented Jan 5, 2016

This PR tries to make the creation of the titles in choropleth graphs more resilient.

I have just copied how titles are built in other graphs, such as rowCharts.

@gordonwoodhull gordonwoodhull added this to the v2.0 milestone Feb 9, 2016
@gordonwoodhull
Copy link
Contributor

Thanks @ferblape. By "resilient", you mean the titles are currently disappearing, or not getting updated?

@gordonwoodhull gordonwoodhull changed the title Append the titles by default Append choropleth titles by default Feb 9, 2016
@gordonwoodhull gordonwoodhull modified the milestone: v2.0 Feb 9, 2016
@ferblape
Copy link
Author

I meant not getting updated.

@mtraynham
Copy link
Contributor

mtraynham commented Jul 6, 2016

I've encountered this before, the code should be .select instead of a .selectAll.

Nesting selections has another subtle yet critical side-effect: it sets the parent node for each group. The parent node is a hidden property on selections that determines where to append entering elements. … There is an important difference between select and selectAll: select preserves the existing grouping, whereas selectAll creates a new grouping. Calling select thus preserves the data, index and even the parent node of the original selection!

https://bost.ocks.org/mike/nest/

Using selectAll here is unbinding the parent node (datum) from updates. For reference, selectAll should almost always be followed by a data function.

@gordonwoodhull
Copy link
Contributor

I looked into this as part of #1239 and added tests for the case where new data objects are supplied for each redraw in aa27858.

I don't get it. It's not using data binding for the title! Here is the current source:

            regionG.selectAll('title').text(function (d) {
                var key = getKey(layerIndex, d);
                var value = data[key];
                return _chart.title()({key: key, value: value});
            });

regionG.selectAll('title').text(function (d) {
var key = getKey(layerIndex, d);
var value = data[key];
return _chart.title()({key: key, value: value});
});

generateLayeredData puts the current data into an object data that only exists for the lifetime of the render or redraw. It uses that data, not the bound data, to fill the regions and update the titles. It binds the map region data to the elements during render, and the crossfilter(ish) data is not bound to elements. The use of selectAll here is apparently intentional to avoid rebinding.

So if anyone can please tell me how the above code does not replace the titles with the current data, I am quite frustrated by this and would desire an explanation.

The only thing I can see might not be replaced is the key from the geoJson data, certainly not the value from the crossfilter data. But this chart does not support replacing the geoJson data with a redraw, you must render again. So I would think there would be a lot more broken than just the titles in that case.

@ferblape, is there any chance you can provide an example (e.g. a block or a fiddle) where titles fail to update? I really need a test for this, to make sure we are doing the right thing.

@ferblape
Copy link
Author

Of course @gordonwoodhull, I'll check the old code and create a block.

@ferblape
Copy link
Author

Sorry for the delay, these dates are a bit complicated.

Thing is that after investigating a bit I've found that the problem was in my code. It's very hard to explain, and I can't create a blocks because the setup it's a bit complicated, and the information we are visualizing is sensitive.

But, as a summary, I was using tipsy.js to display nice tooltips, and I was replacing the attribute title by an element original-title, because otherwise I had two titles, the one from tipsy and the one native from the browser. That was causing something strange that I can't reproduce, but, if I remove tipsy.js and I use the standar title, it gets updated perfectly, without removing any element.

Please, fell free to close and ignore this PR. If you want to test it yourself maybe we could find a way for you to view it in my laptop.

So sorry for the mess, and thanks for the interest!

@gordonwoodhull
Copy link
Contributor

Thanks for your response, @ferblape! That's a big relief because I was afraid there was still another level of complication in these data joins that I didn't know about.

The way the choropleth binds data is odd, and IMO somewhat backwards, but in this case it should avoid the problem that @mtraynham has pointed out in many other charts.

Other folks have run into trouble adding d3-tip to choropleths, and there we determined that the problem was this strange data binding, which makes the map data available but not the reduced crossfilter(ish) data.

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.

3 participants