Applying bigianb's patch as described in issue #789

[r4j4h]

I am not sure if links work through titles, this is in reference to issue #789 . It fixed master for me as well as bigianb.

[gordonwoodhull]

I will apply this soon. I need a test, however. Gotta keep stuff fixed!

Care to give it a shot? It should be something that fails with those crummy numeric ids and succeeds with this fix.

Also, I think the 'id' prefix should be 'chart' or 'dc-chart'. Otherwise it looks good to me.

[r4j4h]

Thanks for the feedback. Yeah good point, I will try to get some tests written tomorrow.

On the prefix, I think 'dc-chart' is the most explicit so I'll go with that one..

Any thoughts @bigianb?

[bigianb]

Thanks for working on this Jasmine. I'm indifferent about the id prefix ... I wonder if there is an advantage in keeping it short though. Are a lot of them created? If so we should keep it short to keep the memory commit down ... if there are not many though then keep it long and descriptive.

[gordonwoodhull]

These ids are per chart. Shouldn't be any memory issue.

(Strings don't generally take up much memory anyway, it's DOM objects you have to worry about.)

[bigianb]

cool - dc-chart it is then.

[r4j4h]

Hey guys, just wanting to mention that I have not forgotten about this, was not able to get around to it over my break. My biggest hurdle is getting the test written, as I am out of the head space and don't even have the project open right now. Looking back through the original issues I see this commit:

9b2a041

@gordonwoodhull for expediency, could I drop in something like that for the test?

[gordonwoodhull]

Ah, great. Thanks @r4j4h, I was just about to ping you about this. I guess #789 is holding a lot of people up from adopting the latest alphas.

I also get writer's block about tests, not having come from that culture, but then once I write them I wonder what the big deal was.

Sure, if you can copy that test and make it fail for the current master and succeed for this PR, that's enough. Perhaps we are simply not seeing #789 in the tests because we always set the id for our test charts and it is enough to add a test (in spec/base-mixin-spec.js ?) that doesn't set the id.

[gordonwoodhull]

Also, you need to modify modify src/base-mixin.js and not dc.js, which is generated.

https://github.com/dc-js/dc.js/blob/master/CONTRIBUTING.md

[gordonwoodhull]

Hmm, looks like the test will have to use one of the concrete charts, not the mixin.

As in the commit you cited, spec/coordinate-grid-chart-spec.js is probably a decent place to put it, even though the functionality tested is in base.

[r4j4h]

@gordonwoodhull So after taking a peek, I think I found a pretty elegant location and test for it in base-mixin-spec.js with some other anchorName tests.

The existing numerical one still passes unchanged as well! I think that it should rather be testing the "auto-incrementing" nature of it's generated fallback ids instead of just that it contains a number, but either way this is a good forward tsep.

[r4j4h]

Sorry to the entire community for taking so long on this :3