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

General discussion and update #1391

Closed
kum-deepak opened this issue Mar 30, 2018 · 51 comments
Closed

General discussion and update #1391

kum-deepak opened this issue Mar 30, 2018 · 51 comments

Comments

@kum-deepak
Copy link
Collaborator

We can use this for general discussion and status. Please put 'discussion' label on this.

@kum-deepak
Copy link
Collaborator Author

  • I am happy with the sunburst chart.
  • In input filter - the name does no go well, please suggest a name, with that change, it will be ready from my end.
  • In check box menu, I need one more round of code review, I have suspicion of some subtle bugs.
  • I will then move onto Add Html legend support #1329
  • In parallel look onto using Firefox for automated tests.

@kum-deepak
Copy link
Collaborator Author

kum-deepak commented Apr 4, 2018

Please let me know the PRs that you would like me to work on, i.e., which can not be merged because of poor test coverage.

I will be less available for next 2-3 days, so, my responses in this period will be slower.

@gordonwoodhull
Copy link
Contributor

Thank you, will do!!

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Apr 6, 2018

PRs which look good but need better test coverage:

@gordonwoodhull
Copy link
Contributor

If possible, #1365 and #1367 both need tests that fail before the changes are applied, but succeed with the changes.

@kum-deepak
Copy link
Collaborator Author

#1365 has test cases, I will check if these fail with old code and pass with the PR :)

@gordonwoodhull
Copy link
Contributor

Fantastic, those look really thorough. I guess it's just #1367 missing tests; I found it a bit confusing but maybe I didn't stare at it long enough.

@kum-deepak
Copy link
Collaborator Author

Thanks for all your support. I understand the approach with smaller single feature PRs. That will help contributors as well :)

In general the approach you are taking is regarding releases is a proper one. It is good to add new features etc., but what users - barring few courageous ones - are looking more is stability.

I have been going through the code and will make successive refactors, however I feel most of those are better taken after we have split the library into modules (like D3). One implicit target for that release will be to allow other users to use contributed charts easily without waiting for it to be merged into main release.

@kum-deepak
Copy link
Collaborator Author

#1414, #1409, and #1416 are ready for your review.

@kum-deepak
Copy link
Collaborator Author

#1418 and #1417 are also ready.

Please give #1418 priority over others - this has overlapping code for renaming selection.

@gordonwoodhull
Copy link
Contributor

Thanks @kum-deepak! Hope to take a look in the afternoon NYC time.

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Apr 20, 2018

I merged 3.0 into develop/master and deleted the branch.

Unfortunately GitHub closed all the open PRs for that branch when I did that.

I recreated the branch and think I reopened all of the PRs, but lmk if anything is still amiss.

@kum-deepak
Copy link
Collaborator Author

Other than #1408 all seems to be ok. Did you close #1408 on purpose?

@kum-deepak
Copy link
Collaborator Author

I am traveling and spending quite less time on my computer. I will be back to my base this Thursday. My responses will be back to normal then.

@kum-deepak
Copy link
Collaborator Author

Should I target all remaining PRs to develop branch?

@gordonwoodhull
Copy link
Contributor

Thanks, I've reopened it! Yes, from now on we'll use the develop branch, standard git-flow. (You could think of the 3.0 branch as a feature branch in that model.)

Good discussion on #1408. I'm taking a few days off myself. Enjoy your travels!

@kum-deepak
Copy link
Collaborator Author

All of PRs opened by me are now based on dc.js/develop. Please try removing 3.0 branch.

Next priority for me will be Sunburst failing on D3v5.

@tttp
Copy link
Contributor

tttp commented Apr 22, 2018

Not sure it's the right place, but I see lots of awesome activities here @kum-deepak
2 new charts I've hacked and with some love could be useful in dc core?

I have no idea how v2 ready they are, just mentioning them so far see if there is a potential interest...

@tttp
Copy link
Contributor

tttp commented Apr 22, 2018

And I'm abusing again this issue to mention something we might tackle for v3: cleanup the css and split what is "100% needed" vs "choices that made sense but are fighting the site layout"

I'm assuming most of the dc users are building their site on an existing framework (bootstrap, Materialize...) and some of the default css from dc are not playing nicely (eg. dc-chart float).

By cleaning up the css and only keep what is purely svg chart styling, it would be easier to put charts into an existing grid... and holy grail having a responsive solution out of the box (we aren't that far away, check tttp.eu for example)

For compatibility reason, having a separate "layout.css" that contains the removed css rules would probably help the migration...

Thought?

@gordonwoodhull
Copy link
Contributor

Thanks @tttp. Besides the float left #673, what else would go in layout.css?

@kum-deepak
Copy link
Collaborator Author

@tttp for both of charts (datatable, wordcloud) please submit PRs. I can help you in making these ready for updated dc and d3. One of the targets post DCv3 is to allow additional usage of additional charts (or using enhanced version of a builtin chart) without those getting released as part of DC. So, additional chart types are definitely welcome.

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Apr 22, 2018

Those other charts sound useful, and they are frequently requested.

But rather than write and maintain our own version of datatables.js, why not figure out how to properly integrate with it #966? Every time someone asks about it, I ask if they will contribute back their solution, but no one does. Apparently it's not all hard but it may require some ugly hacking. (I am guessing; I haven't tried it myself but I have asked at least 5 times and no response).

EDIT: or maybe that's what you're suggesting, since you were very active on that ticket?

@tttp
Copy link
Contributor

tttp commented Apr 23, 2018 via email

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Apr 23, 2018

@tttp, why did you find you couldn't use DataTables? Is it the multiple tbody problem #1164?

I'm wary of pulling in something that duplicates so much functionality - datatables.js is huge (81K minified) so we'd always be playing catch-up.

@gordonwoodhull
Copy link
Contributor

@kum-deepak, thanks for moving the PRs. I successfully deleted the 3.0 branch without autoclosing any PRs. (AFAICT - there aren't any notifications when a PR is closed for this reason, so I had to compare screenshots of the PR page!)

@kum-deepak
Copy link
Collaborator Author

All looks good.

@tttp
Copy link
Contributor

tttp commented Apr 29, 2018

Hi @gordonwoodhull, the main problem was that datatable performance isn't that great when you have a lot of rows, and a lot of the features they implement is basically crossfilter.

with textinputfilter you have the filter adding pagination+sort is a handful of lines

@gordonwoodhull
Copy link
Contributor

Thanks @tttp.

I see, that's frustrating. Is the problem that you still have to generate a huge table, then it parses the html back into data and only displays part of it?

@kum-deepak
Copy link
Collaborator Author

I realized that you have tagged the 3.0.0 release on npm. Congratulations!!

@gordonwoodhull
Copy link
Contributor

Congratulations to you too @kum-deepak!

3.0 has the charts and widgets that were ready with substantial tests, that you helped port.

Now we're at a fork in the road. If you still have time and want to help out with dc.js, do you want to...

  1. Bring in more charts, fixing up PRs based on feedback and writing more tests where they are lacking? (General discussion and update #1391 (comment))
  2. Take on tough lingering bugs like the composite ones that your Alternate strategy for brushing in composite charts #1408 potentially can fix?
  3. Refactor proposals like Composite Pass-Through Properties + Rescale #1365 that provide much needed features but which would make the dc.js code harder to understand and maintain? There are similar refactorings that could happen inside the library, like replace dc.utils.toHierarchy with d3.stratify #1429 d3.stratify.
  4. Modularize dc.js into dozens of little libraries for dc.js 4.0, a vast effort but mostly mechanical and already figured out by D3?
  5. Work on something else you think is important?

I'm here to guide you, should you choose to continue. If you have to move onto other things, I understand that too. This port was a huge contribution and accomplishment!

@kum-deepak
Copy link
Collaborator Author

Thanks @gordonwoodhull! Your guidance has been really helpful.

I will like to take up all of the above. 👍

In addition I will like to add the following (some of which may already be possible):

  • Add ability to save and reapply filtered state (I will write a proposal) of a chart group.
  • A proper example of working with large remote data (I will write a proposal).

I would suggest to complete modularizing dc.js before bringing in more charts. Other items can be taken in parallel.

Happy to let you prioritize the sequence.

@zach-sim
Copy link

zach-sim commented May 1, 2018

I have a library I use in a few projects to add filter saving and applying to dc. https://github.com/umbra268/dcFilterRememberer

It doesn't apply to only specific chart groups as it does it on all charts in the chart registry and it allows you to use the browser's back and forward buttons to undo/redo filters.
It does have issues if you're dynamically creating and removing charts so isn't a 100% fit.

@mukherjeea
Copy link

is the sunburst chart in 3.0.3 complete? I see it renders when the group keys are reduced to a single value. However when it's reduced to multiple values and I use a value accessor to use one of them it fails.

@kum-deepak
Copy link
Collaborator Author

@mukherjeea please open a new issue with a link to example (jsfiddle or similar). Thanks!

@mukherjeea
Copy link

Just created a new issue.
#1440

Thanks in advance!

@tttp
Copy link
Contributor

tttp commented Jul 9, 2018

Hi @gordonwoodhull,

Yes, with datatable, you need to feed it with all the rows so it can then do the filtering and sorting of the rows.

https://github.com/TechToThePeople/mep/blob/master/index.html#L494
Sorry @kum-deepak , I missed your "PR welcome", I'm creating a new issue: #1465

@gordonwoodhull
Copy link
Contributor

Thanks @tttp. Seems like this option should help performance, but I don't know if it helps enough:

https://datatables.net/examples/data_sources/js_array

You're right that it's duplicating a lot of crossfilter functionality, but I expect the larger performance problem is generating a lot of html and then parsing it. DOM slow, JavaScript fast.

Seems like we could remove dc.dataTable from the equation and write a plug-in that just fetches the data and feeds it to datatables.js

@tttp
Copy link
Contributor

tttp commented Jul 9, 2018 via email

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Jul 25, 2018

I started a new project https://github.com/dc-js/dc.datatables.js

It's just the skeleton of the idea, but it mimics the interface of dc.dataTable and uses jquery.dataTables.js for rendering. So it has pagination and search "for free", so to speak, and in theory it would provide all the features of jquery.dataTables.js, which is a very rich library.

Here's an example, swapping out dc.dataTable for dc_datatable.datatable in the stock example. This is only 6724 rows but it seems plenty fast.

@tttp or anyone else, I'd be curious if this works for your use-cases. If so, please consider contributing, since this is just a bare-bones implementation. Some features from dc.dataTables are disabled or missing; others aren't relevant.

@Ushaaradhya
Copy link

Hi gordonwoodhull / deepak,

I am trying to recreate the download-table.html. I am not able to do so. I get only the buttons for download and none of the charts are populating. I am new to d3 and not much aware of what is the issue. Can you please let me know if it is a browser issue or any other issue?
Uploading the image of the html page for your reference.

Appreciate your help ASAP.

Thanks,
Usha

image

@gordonwoodhull
Copy link
Contributor

gordonwoodhull commented Aug 27, 2018

Hi @Ushaardhya,

Do you mean this page does not work?

http://dc-js.github.io/dc.js/examples/download-table.html

Or do you mean that when copying the files to your own server it doesn't work?

Any errors in the browser console?

@Ushaaradhya
Copy link

Hi gordonwoodhull,

Copying files to my server is not working. There are no errors in the console.
only those buttons are displayed, no charts are displayed.

Attaching the screen shot for the same.
Let me know if u need any thing else.

Thanks,
Usha

image

@gordonwoodhull
Copy link
Contributor

I was asking about the browser console.

Open up the developer tools in your browser and look at the console there. I suspect you will see some error there.

@Ushaaradhya
Copy link

This is the error it is showing

image

@Ushaaradhya
Copy link

Please help me on this error.

image

@gordonwoodhull
Copy link
Contributor

I see, thanks @Ushaaradhya. That explains a lot.

It looks like some code in header.js that is used to generate the links to the source in the header of the example is breaking because you have a different directory structure. In order to get it working, you could change the first line that is breaking to just

var dir = 'examples';

In general, I don't really expect people to use the examples as-is. The intention is to provide example code rather than a starting point for your application. But it would be a good idea to make the code more resilient for cases like these.

@Ushaaradhya
Copy link

Thank you very much @gordonwoodhull for the reply.
As I told you I am very new to d3 and does not have a prior knowledge of JAVA aswell.
I will modify the code to our need once it is running.

@sgavathe
Copy link

sgavathe commented Dec 6, 2018

https://dc-js.github.io/dc.datatables.js/ this sample not working in IE 11. In order to get it working with your sample, do i need to downgrade DC.JS to lower version?

@gordonwoodhull
Copy link
Contributor

Thanks for the report @svgavathe, I've created an issue on the repo, linked above. dc.js 3.0 and dc.datatables.js should still be compatible with IE, but probably some ES6 crept into the code somewhere.

@gordonwoodhull
Copy link
Contributor

Ah, got it, yes, it is a general dc.js issue, not just about dc.datatables.js.

http://dc-js.github.io/dc.js/ fails as well.

D3v5 introduces dependencies on some ES6 APIs. See #1507 for details.

@gordonwoodhull
Copy link
Contributor

This ticket was specifically for discussion of dc.js 3.0. IMO there is no need for a "general discussion" ticket, although this did attract a lot of random questions and discussion. 😄

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

No branches or pull requests

7 participants