intellipharm-pty-ltd / dc-addons

dc.js chart addons
http://intellipharm.github.io/dc-addons/
MIT License
67 stars 27 forks source link

dc.leafletMarkerChart: Deselected markers aren't visually distinct from selected markers #19

Closed jasonnance closed 8 years ago

jasonnance commented 8 years ago

On other dc charts, when applying a filter by clicking on an element on a chart, all other elements on the chart become "deselected", or grayed out (unless they were part of an existing filter). This behavior isn't implemented by the marker chart, so it's hard to tell which markers are currently selected if the user applies a filter on the chart.

I realize implementing this behavior is probably complicated by all the different options available for display/filtering on the marker chart, but I need to make it work for our use case (no clustering, just click filtering on the markers). I can file a PR with the changes if you'd like a start in making it work in general.

Tim-Intellipharm commented 8 years ago

I am not sure if I am understanding you correctly or not but instead of hiding the markers on filtering you want them to be grayed out?

Check out dc.leafletCustomChart with the example being at http://intellipharm.github.io/dc-addons/examples/leaflet-custom.html. You have to manually create the markers in the renderItem function, but then in the redrawItem you could change the marker icon to a gray color if it is filtered.

Does this help you with your solution?

jasonnance commented 8 years ago

Not quite -- sorry, I'm not explaining this well. I'm not referring to how the chart looks when other charts filter it; I'm referring to how it looks when it filters itself.

Take the main dc.js site as an example: https://dc-js.github.io/dc.js/

If you click the bar labeled "Mon" on the "Day of Week" row chart, all the other bars on the chart become grayed out. It lets you see immediately what you've filtered on that chart. Currently, the leafletMarkerChart doesn't behave like that; if its current behavior were transferred to the row chart, the other bars on the row chart would stay colored when "Mon" was clicked.

The approach I took was just dropping the opacity of each marker to 0.3 when deselected (and bringing it back up to 1.0 when selected or the filter on the chart was cleared). Unfortunately, I can't really put up a live example, but here are some screenshots that hopefully make it clearer:

Current behavior: Before and after clicking on the marker in Minnesota: screen shot 2016-07-19 at 6 14 58 pm

Suggested behavior: Before clicking on the marker in Minnesota: screen shot 2016-07-19 at 6 14 58 pm After clicking on the marker in Minnesota: screen shot 2016-07-19 at 6 16 25 pm After additionally clicking on the marker in Arizona: screen shot 2016-07-19 at 6 16 57 pm

And so on. Does that make more sense? The actual changes are here: https://github.com/rtidatascience/dc-addons/commit/4db44a81a9451bfb41fc736051544d3e97fe9a35#diff-c73d4bbe95af63eaefbb34069192fc7d

Tim-Intellipharm commented 8 years ago

That code looks fine, but I believe using dc.override(_chart, 'filter' may be unnecessary as the selectFilter function calls _chart.filter which would then call the override. Could you try putting the logic into the selectFilter function?

If so I would be more than happy to merge a pull request. Otherwise I will try to look into it when I get time.

gordonwoodhull commented 8 years ago

A couple of suggestions. (I maintain another fork of the dc.leaflet stuff.)

jasonnance commented 8 years ago

I would be glad to submit a PR and also to improve the code!

@Tim-Intellipharm -- I tried putting the logic in selectFilter at first, but that function is only called when a marker is clicked; in order to set the opacity back to normal when the filter is removed, we need to also catch the case where the user clicks elsewhere on the map to reset its filter, or even when a "reset" button elsewhere resets the chart filter by calling .filterAll(). I wasn't sure how else to do it other than overriding `_chart.filter'.

@gordonwoodhull -- to your first point, that makes more sense; looks like I got a little confused trying to copy the way filter was overridden in some other dc charts/mixins. To your second point, I'm not seeing how to make it work that way; the chart doesn't get redrawn when its own markers are clicked. To clarify, I'm not trying to change the behavior when the chart gets filtered by other charts. I'm fine with the disappearing markers, as that's consistent with the way the other dc charts behave. I want to make it consistent with, for example, the way row charts apply the 'deselected' class to rows that aren't part of the chart's current filter, as currently all markers look the same regardless of the chart's current filter. I just wasn't able to find a convenient hook, like the row chart's drawChart function, to add the logic. But, as you can see, I'm not too familiar with the dc code base, so I may be missing something.

gordonwoodhull commented 8 years ago

Hmm, I would definitely expect both events to trigger a redrawAll or redrawGroup. It looks like selectFilter triggers one, and it looks like the click-on-background event is in the zoom handler and also causes a redraw. But I'm not in the debugger right now, so you'd know better if these are actually getting triggered.

jasonnance commented 8 years ago

Ah, it looks like the chart is being redrawn in both cases, but the redraw function returns before touching any markers if the chart's groups haven't changed (https://github.com/Intellipharm/dc-addons/blob/08c38efa207d21338a4ddfeda807558ef8f48ebf/src/scripts/leaflet-marker-chart.js#L81-L82).

I tried taking out that early return and doing as you suggested with regard to setting opacity during redraw (although createmarker is only called for new markers by default, so I think it has to be done directly in _doRedraw). It does end up being much simpler: https://github.com/rtidatascience/dc-addons/commit/2ec858ec653d2518cbf1da8a6dd661ce0065cf55#diff-c73d4bbe95af63eaefbb34069192fc7d

Tim-Intellipharm commented 8 years ago

@jasonnance If you would create a PR with just the changes in the src folder I would be happy to merge it in.

fletcmd commented 6 years ago

is anyone going to document the results? I'm not exactly sure how to use this feature still.