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

Checkbox filtering dc v3 #1389

Closed
wants to merge 9 commits into from
Closed

Checkbox filtering dc v3 #1389

wants to merge 9 commits into from

Conversation

kum-deepak
Copy link
Collaborator

Based on #1348 - updated for D3v4.

@kum-deepak
Copy link
Collaborator Author

Facing a peculiar issue - few test cases fail in phantomjs, but succeed when I run in browser (firefox, safari, chrome) on a Mac. Issue appeared only in current commit, works well with D3v3 (original PR).

@gordonwoodhull
Copy link
Contributor

Ugh, that is upsetting. Perhaps phantomjs is falling behind on compatibility with the browsers. I believe it has been abandoned and people have moved on to other headless browsers for testing.

Perhaps it is time for us to do the same? How do you feel about trying some infrastructure upgrades?

The particular error is

   TypeError: Unable to delete property. in file:///home/travis/build/dc-js/dc.js/web/js/d3.js (line 1303)
   file:///home/travis/build/dc-js/dc.js/web/js/d3.js:1303:37
   selection_each@file:///home/travis/build/dc-js/dc.js/web/js/d3.js:1185:41
   selection_property@file:///home/travis/build/dc-js/dc.js/web/js/d3.js:1310:18
   _doRedraw@file:///home/travis/build/dc-js/dc.js/dc.js:11619:26
   redraw@file:///home/travis/build/dc-js/dc.js/dc.js:1849:38
   redrawAll@file:///home/travis/build/dc-js/dc.js/dc.js:369:25
   redrawGroup@file:///home/travis/build/dc-js/dc.js/dc.js:1900:25
   file:///home/travis/build/dc-js/dc.js/dc.js:11725:31
   trigger@file:///home/travis/build/dc-js/dc.js/dc.js:972:16
   onChange@file:///home/travis/build/dc-js/dc.js/dc.js:11724:26
   file:///home/travis/build/dc-js/dc.js/spec/cbox-menu-spec.js:191:27

I could be wrong, but I am guessing this means that d3.selection.property has changed its implementation somehow for the case where a property should be deleted (when the value is null).

I believe there's only one line that could trigger this, here:

                .property('checked', function (d) {
                     return d && _chart.filters().indexOf(String(_chart.keyAccessor()(d))) >= 0;
                 });

https://github.com/dc-js/dc.js/pull/1389/files#diff-7dc7e6020cb53b43b5397330e4dc33d2R76

So you could try adding || false in case d is null.

@kum-deepak
Copy link
Collaborator Author

@gordonwoodhull you are a genius 👍

I am creating a new task for infrastructure upgrades.

@kum-deepak
Copy link
Collaborator Author

Complete my work for now. Moving onto next PR.

@kum-deepak
Copy link
Collaborator Author

Hi @ialarmedalien, many thanks for getting back! Your assistance will be quite appreciated. So, far I have just ensured that code works with newer version of D3. In the meanwhile it will be helpful if you can check that functionally and test wise it mimics the select menu component.

I will give commit access to the branch so that you can directly push your changes.

I am less available for next 2-3 days, so my responses will be slow.

@kum-deepak kum-deepak changed the base branch from 3.0 to develop April 22, 2018 08:12
@kum-deepak
Copy link
Collaborator Author

Rebased and changed base to dc.js/develop.

@gordonwoodhull
Copy link
Contributor

Thanks @ialarmedalien, @kum-deepak!

Merged for 3.0.0

@kum-deepak kum-deepak deleted the checkbox-filtering-dc-v3 branch April 28, 2018 18:19
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.

2 participants