markmarkoh / datamaps

Customizable SVG map visualizations for the web in a single Javascript file using D3.js
http://datamaps.github.io
MIT License
3.78k stars 1.01k forks source link

updateChoropleth bug when subunit names don't make valid selectors #127

Closed ramnathv closed 8 years ago

ramnathv commented 10 years ago

Not sure if this is fixable in datamaps, but thought I should bring it up anyways. One of the users of rMaps noticed that weird things happen when subunit names have certain characteristics. For example, consider nord - pas-de-calais. When updateChoropleth was called with this as one of the subunit names, it errored out, and moreover did not complete the updates for all records that occured after this one.

More investigation revealed that the trouble is that d3.select('.' + subunit) errors out, since it does not see a valid query selector. So my questions is, can updateChoropleth and datamaps sanitize subunit names in a way that will lead to more robust behavior, or is this better left to the user.

Note that several times the limitation is not the CSV file, but the geoJSON file one can get their hands on. So user updates have to happen in both, which is why I thought it would be easier to handle in datamaps itself.

markmarkoh commented 10 years ago

This is the second issue reported about the updateChoropleth selectors.

This is addressable in datamaps. I think the approach here is to add a data- attribute, like data-geography-id, which can contain spaces, and select with d3.select('[data-geography-id="nord - pas-de-calais"]'), which should work fine.

My only concern is performance since class selectors are 40% more performant than data- attr selectors

ramnathv commented 10 years ago

Performance is an important concern. Here is another idea. How about running all subunit names through a sanitization function that will define the class. For example, a simple function could just replace all spaces with underscores. It will be a reversible transformation.

markmarkoh commented 10 years ago

That's a good idea, and it's something that's pretty close to some common d3.js patterns.

The default would basically be:

function id(geoID) {
    return geoID;
}

A user could override it in an option, like they can setProjection.

var map = new Datamap({
   element: getElementById('main'),
   id: function(geoID) {
     return geoID.replace(' ', '_');
   }
});

And for updateChoropleth as well as the initial painting, I would just run every id through the id() function.

markmarkoh commented 10 years ago

I don't think it being reversible is necessary, or at least I can't think of a use for it at the moment.

ramnathv commented 10 years ago

Nice idea. I agree that reversibility is not a concern, since you are not overwriting property names. I was initially thinking that it is necessary to retrieve names for display in popupTemplate. The id function should do the trick. Can we think of a more descriptive function name?

ramnathv commented 10 years ago

And, I am thinking that the default should be geoID.replace(' ', '_') since spaces are a very common pattern in names.

WunderBart commented 9 years ago

What about countries without ISO ID (the -99 issue)? World topojson contains actually 3 of them, which leads to 3 identical IDs for the svg collection. I can modify source manually but it would be nice to have some fallback in updateChoropleth for such situation.