search

personality-insights / sunburst-chart

A visualization for IBM Watson Personality Insights service output.
https://personality-insights.github.io/sunburst-chart/
Apache License 2.0
43 stars 37 forks source link

WIP: upgrade d3 to version 4 #31

Closed soumak77 closed 6 years ago

soumak77 commented 6 years ago

Update d3 logic to use version 4 API. There are still issues with displaying the sunburst that need to be worked out.

I added a script to allow viewing changes to the sunburst in the browser. Use the following command to run the build and view the changes:

npm start

Most of the conversion from version 3 to version 4 was straight forward. The one area I am uncertain about is converting the partition logic: https://github.com/personality-insights/sunburst-chart/blob/master/src/personality-chart-renderer.js#L555-L574

UPDATE: PR now supports using both version 3 and version 4 (defaulting to version 3 for backwards compatibility). The d3 dependency has been moved to a dev dependency so that v4 users will not need to download v3 as well. This will not affect users including d3 in their html (as shown by the example). Those including it in their javascript bundle will need to explicitly add d3 to their list of dependencies (which is likely already done).

IMPORTANT: The file src/personality-chart-renderer.js has moved to src/d3-renderers, but I will keep it in the PR for now to make the diff easier to read. It should be removed before merging this PR.

soumak77 commented 6 years ago

Is anyone in the community an expert with d3? I have done what I could to convert to using d3 version 4, however, I am stuck trying to get the sunburst to display properly. If someone could provide me with some tips or sync my PR and get it to work that would be awesome! I feel like it is really close to fully working.

soumak77 commented 6 years ago

@germanattanasio would anyone on your team be able to spend some time getting this to work?

germanattanasio commented 6 years ago

@aprilwebster can you look at this?

aprilwebster commented 6 years ago

@germanattanasio sure, I can take a stab at it.

soumak77 commented 6 years ago

I've updated the PR to support using both version 3 and version 4 (defaulting to version 3 for backwards compatibility).

The file src/personality-chart-renderer.js has moved to src/d3-renderers, but I will keep it in the PR for now to make the diff easier to read.

soumak77 commented 6 years ago

I've integrated these changes into my angular app and exposed APIs to make it easy to update the sunburst on data changes without needing to re-create the entire SVG. D3 version 4 support is the last big thing needed before this library is near perfect!

germanattanasio commented 6 years ago

This is looking solid @soumak77!

soumak77 commented 6 years ago

thanks @germanattanasio! By the way, all the examples can be reduced to 1 file at some point (just needs a touch of jQuery). Started off as only 2 files then snowballed when I added localization and smaller bundles.

soumak77 commented 6 years ago

@germanattanasio fyi, tests will pass once a new version of personality-trait-names is published as the pending changes rely on the new standalone modules

germanattanasio commented 6 years ago

I updated all the other libraries on npm.

germanattanasio commented 6 years ago

If you move the example to the docs folder, I can use that as GitHub pages and have some live documentation that shows how the chart is re-rendered. We won't call Personality Insights but we can have a text-area where you paste the profile.

What do you think?

soumak77 commented 6 years ago

@germanattanasio that would be awesome! The PR can be merged at any point as the changes are fully backwards compatible. Hopefully someone with D3 expertise can take a look soon and get version 4 working.

germanattanasio commented 6 years ago

Can you update this PR with what is left so that I can see if someone has the knowledge to continue this.

soumak77 commented 6 years ago

@germanattanasio PR is ready. As mentioned in my original comment, src/personality-chart-renderer.js will need to be removed once the PR is merged (or I can do it before). It is just there now to make the diff easier to read.

germanattanasio commented 6 years ago

Let's remove and I will merge this and release it

soumak77 commented 6 years ago

@germanattanasio done

soumak77 commented 6 years ago

@germanattanasio I just noticed, the docs will need to be updated to contain 2 different index files, one for D3 version 3 and another for D3 version 4 so that we can include the correct versions of D3 separately. I'll make this change now.

germanattanasio commented 6 years ago

@soumak77 you need to make sure everything that the page needs to load is in the docs folder.

Try to also use https://cdnjs.com/ for d3 and jQuery. We can use unpkg for the sunburst lib.

soumak77 commented 6 years ago

@germanattanasio docs should be good now. There are 2 files that will be used for github pages: index.html and index-d3v4.html. The other 2 files (index-test.html and index-d3v4-test.html) are used for local testing by running the npm start command.

germanattanasio commented 6 years ago

I think https://unpkg.com/personality-sunburst-chart@2.2.0/lib/all.js needs to be https://unpkg.com/personality-sunburst-chart@2.2.0/lib/index.js

soumak77 commented 6 years ago

@germanattanasio sure, feel free to rename

soumak77 commented 6 years ago

@germanattanasio want me to submit a PR for that?

germanattanasio commented 6 years ago

yeah. You did all the work already

germanattanasio commented 6 years ago

You should also close the issues you fixed.