Updated a number of components to add useful features

[MHeisoku]

This is my first time making a pull request and I should note that I haven't minified the js and haven't regenerated js.map files.

The features I have added are in the readme and the examples have been updated to demonstrate some of the key ones. Being able to use your own pre-existing Leaflet map object is one of the most powerful and ensures that DC.js is able to seamlessly plugin to existing maps.

Happy to discuss and revise things further - keen to see this library mature as it is really beneficial for interactive maps. I have some dynamic choropleth and legend regeneration code I'd like to refactor in a future version - replacing an existing with new layer and also replacing the legend is critical to some intuitive discovery use cases.

[gordonwoodhull]

Hi @MHeisoku, these improvements look great!

I don't normally like to merge a lot of unrelated changes in a single PR or commit, but since you are a first-time contributor, and this is great stuff, I am willing to take this as-is.

Would you be willing to summarize the individual changes for the changelog, and to help my review?

Thanks!

[MHeisoku]

Yes can do. I can summarise each change in bullet point form if you'd like?

[gordonwoodhull]

Yes, that would be perfect, thanks!

It doesn’t need to be detailed - take a look at Changelog.md for guidance.

[MHeisoku]

Is this sufficient? Let me know if it hasn't hit the mark. I have more changes made to choro, bubble and marker to add features but will leave them to separate pull requests for each. Let me know what I can do for next steps for this PR.

0.5.2

• leafletBase now supports reuse of an existing leaflet map object using the .map() function
• new legend function legendTitle() added to optionally specify a title that appears above the legend contents if set
• bubbleChart changed to inherit from ColorMixin so .colors(), .colorDomain() and .colorAccessor() can be used to change bubble colors and a legend can be added to the map to show color ranges
• markerChart changed to allow you to define whether titles should be shown or not - function showRenderTitle() replaces prior default of rendering title at all times
• new markerChart functions fitOnRender() and fitOnRedraw() added to allow map to fit to bounds on initial render and/or crossfilter redraw respectively

[gordonwoodhull]

That's perfect, thanks very much.

I'm just recovering from illness, will review soon!

[gordonwoodhull]

These features look super helpful. Thanks also for providing an example of each feature in one of the demo charts - that's the best way to test things in this library, in lieu of proper tests, and it's helpful to users.

When you talk about using .map() to "plugin to existing maps", do you mean adding a layer to a map which is managed by other code in the page?

Please try to make changes backward-compatible, so that people can upgrade without too much effort. I tested backward compatibility of the bubble chart colors by commenting out the color-related lines

          // .colors(colorbrewer.Reds[7])
          // .colorDomain([
          //     d3.min(facilitiesGroup.all(), dc.pluck('value')),
          //     d3.mean(facilitiesGroup.all(), dc.pluck('value'))
          // ])
          // .colorAccessor(function(d,i) {
          //     return d.value;
          // })

and got a messy chart:

It doesn't have to work the same way as before, with grey unselected bubbles, but

1. The default color scale should probably be continuous, not categorical
2. Random colors are probably not helpful - perhaps the bubble chart should change the default color accessor to use the value accessor, instead of the key as dc.ColorMixin has by default

It looks like unselectedColor is changing from a color value to a function, and you've documented the change, which is great. However it's not clear from the docs what the new function does.

1. Rather than saying it "can render from a color domain (like a choropleth)", better to say that the default handler takes the datum and passes it to ColorMixin.getColor()
2. I don't think calling the value accessor and passing the result as the second parameter of ColorMixin.getColor is correct. If you don't have the array index in the group data, probably better to omit the second parameter.

I tested fitOnRender() by commenting out the center and zoom for the two marker charts. As expected, the first chart worked and the second chart didn't. Please remove center and zoom from the first example (for a better demo of what that feature is about), and document fitOnRedraw().

In future PRs, please don't check in generated artifacts such as dc.leaflet.js. Those get generated on release, and they make merging a little messier.

Great work! I'm glad to see this library getting some major upgrades. I look forward to seeing your "dynamic choropleth and legend regeneration code" once we have reviewed and merged this.

[MHeisoku]

Thanks for reviewing and for the comments Gordon - helps me to know what to look out for next time. Can I amend this PR and resubmit for review or do I need to close and submit a new PR? Sorry for such basic questions.

[gordonwoodhull]

Just push more commits to the same branch and they will show up here. A PR is basically a live diff between two forks/branches.

Another tip for next time: when doing multiple PRs, you will find that you want to dedicate a branch for each PR, instead of using master.

[gordonwoodhull]

Hi @MHeisoku, are you still interested in contributing?

These are great improvements and I'd really like to see them merged!

[MHeisoku]

Apologies Gordon. Have been busy wrapping up a job and moving onto another recently and haven't had the time until recently to commit changes. See latest changes which hopefully address your feedback. I have also demonstrated how to use the .map() setter function in leafletBase. I found the .map setter function useful to preserve Leaflet's responsive behaviour and allow a lot of Leaflet specific code and plugins to initialise a map before dc layers are added.

From now on I will stick to isolating PRs to either a particular chart type, or new/enhanced functionality that is common across chart types. I've also figured out how to use grunt (pretty cool and makes changing and testing really easy) and will make sure in future I only push files in src so you can generate dc.leaflet.js as part of the build process.

[MHeisoku]

Hi Gordon. Just want to check to see whether you need me to make any further changes?

[gordonwoodhull]

Thanks @MHeisoku! Sorry for the slow review - I was caught up in a few other projects this week.

I tested backward compatibility by temporarily checking out the old index.html. There is a small aesthetic difference in the bubble chart but everything works properly now.

Before

After

IMO it looks better without the bold outlines, so I think this change is fine, just wanted to document it for posterity.

I removed the comments you made for testing, as I find it confusing to have "what-ifs" in the code. Also it's not a good practice to set a scale inside an accessor, although it does work. I guess you did this for brevity.

I also added your setContent change to the changelog.

Released as 0.5.2!