Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Propagate filters on composite chart to children fixes #677 and #706 #1435

Closed
wants to merge 4 commits into from
Closed

Propagate filters on composite chart to children fixes #677 and #706 #1435

wants to merge 4 commits into from

Conversation

kum-deepak
Copy link
Collaborator

Final change is quite small.

All filter related calls eventually call .filter, so covering just one case proved to be enough. Got a simpler code by listening to 'filtered' event and replacing filters on children. Overriding was getting messy because coordinate mixin already overrides filter.

@gordonwoodhull
Copy link
Contributor

Seems too simple somehow! 😄

If this is correct, then compositeChart.applyBrushSelection can be eliminated too.

.on('filtered', ... should definitely be namespaced because users will listen to it too.

I'll try to devise more tests / examples.

@kum-deepak
Copy link
Collaborator Author

kum-deepak commented May 13, 2018

Thanks @gordonwoodhull, I had not realized that compositeChart.applyBrushSelection has actually become redundant 😃

Also, had not realized behavior of d3.dispatch with same event names - changed in composite chart, also changed another similar usage in coordinateGridMixin.

I think additional test can be added to one of the existing test groups for setting/removing filter(s) and checking in children. Would you like to take a shot?

Adding an example would definitely be cool.

@gordonwoodhull
Copy link
Contributor

I'll see what I can do when I merge this later this week. Need to rest today.

Until then, any further tests or examples are welcome!

@kum-deepak
Copy link
Collaborator Author

Added few additional test cases. I think these should cover the updated functionality.

@kum-deepak
Copy link
Collaborator Author

I noticed that #1366 also fixes the 'filtered' listener in coordinateGridMixin. I think the code in this PR is more consistent with rest of the dc. However it may be worth picking up the test case from #1366.

@gordonwoodhull
Copy link
Contributor

Ah, thanks for pointing that out! I'll merge that along with this.

@gordonwoodhull
Copy link
Contributor

Thanks @kum-deepak! Merged for 3.0.4

Amazing how simple this turned out to be, after all the other fixes anyway.

I went with namespacing the event with dcjs-range-chart and dcjs-composite-chart. Rationale is that we still want the user to be able to guess the namespace and remove/replace the listener if they need to.

The unique ids we use for charts in a page, but there is no risk of the charts interfering with each other here.

I didn't manage to write any more composite chart examples today. Hope to return to that another day!

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

Successfully merging this pull request may close these issues.

2 participants