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

Sync highlight between multiple bar charts #682

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Sync highlight between multiple bar charts #682

wants to merge 1 commit into from

Conversation

dimirc
Copy link

@dimirc dimirc commented Aug 24, 2014

WIP

Demo plunker

Related to #681

@dimirc
Copy link
Author

dimirc commented Aug 24, 2014

@gordonwoodhull what do you think of this approach?

if (!arguments.length) return _syncGroup;
//TODO Check if it isn't already added
_syncGroup = _;
_syncGroup.push(_chart);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In current implementation, user needs to send an array, but maybe will be better if they just send a string and we could have central registry for this groups.

@vprus
Copy link
Contributor

vprus commented Aug 28, 2014

Hi Wladimir,

I've tried your patch - since I'm using row chart, and not bar chart, I had to manually apply the same changes to row chart code. It worked fine!

On nit was that I had to use

    var filter = _chart.keyAccessor()(d);

in the onClick method, with d and not d.data. I actually wonder whether we can have a new mixin for this, or otherwise avoid copy-paste?

Also, maybe this behaviour should be automatic? I.e. should not all charts using the same dimension be part of the same sync group automatically?

@gordonwoodhull
Copy link
Contributor

Yeah, I think this is very close to the right solution. Note that the same problem arises with composite charts, which are another form of charts sharing the same dimension. See #390 and linked issues.

As to @vprus's question whether the behavior should be automatic: that sounds reasonable (as long as there is a way to opt out). However, off the top of my head, I can't think how to implement it, because dc.js doesn't maintain a directory of dimensions and their associated charts. A hacky way would be to actually annotate the dimension object, but that's pretty weird and would undoubtably cause problems for people who are stubbing out crossfilter with some other backend.

@gordonwoodhull
Copy link
Contributor

Sorry for the slow reply.

I've started reviewing this. I don't know what it is about the dc.js zeitgeist, but once someone asks for a feature, everyone is asking for it.

Some comments:

  • lift this to the base chart, so that it can work for different charts and even between chart types.
  • add an internal method to update the filters without setting the dimension filter. Changing .filters() is hacky, and if we just set the filters then there's no need for the splice/indexOf stuff.. and it will work for all filter types. I think this solution only works for ordinal (toggle) filters, not for ranges or the various other
  • it would be cleaner to just call the filter function for "this" chart, and then skip "this" chart when looping, rather than using the filterOnce flag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants