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

Use c3 instead of nvd3 #3571

Closed
floreks opened this issue Feb 14, 2019 · 12 comments · Fixed by #3590
Closed

Use c3 instead of nvd3 #3571

floreks opened this issue Feb 14, 2019 · 12 comments · Fixed by #3590
Labels
kind/feature Categorizes issue or PR as related to a new feature.

Comments

@floreks
Copy link
Member

floreks commented Feb 14, 2019

nvd3 is not maintained for a long time and uses a very old version of d3. We should switch to something that is maintained and frequently updated, i.e. c3.

@floreks floreks added the kind/feature Categorizes issue or PR as related to a new feature. label Feb 14, 2019
@k8s-ci-robot
Copy link
Contributor

@eloyekunle: GitHub didn't allow me to assign the following users: eloyekunle.

Note that only kubernetes members and repo collaborators can be assigned and that issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@maciaszczykm
Copy link
Member

@eloyekunle Will you be working on it?

@eloyekunle
Copy link
Contributor

@maciaszczykm Yes I will.

@maciaszczykm
Copy link
Member

Great :)

@maciaszczykm maciaszczykm added this to the v2.0.0 milestone Feb 21, 2019
@eloyekunle
Copy link
Contributor

I'm wondering if we shouldn't use d3 directly instead of using wrappers (like nvd3 and c3).

@maciaszczykm
Copy link
Member

@eloyekunle Would be the best IMO. Is it much effort?

@eloyekunle
Copy link
Contributor

We might have to create internal utility classes for the charts we'll need, so it's moderate effort.

@floreks
Copy link
Member Author

floreks commented Feb 22, 2019

We might have to create internal utility classes for the charts we'll need, so it's moderate effort.

That's what I was trying to avoid, because then maintenance effort is bigger. Why is using some kind of framework not better?

@maciaszczykm
Copy link
Member

@floreks In my opinion, it is good to have our own implementation if there is not too much of it. We can control everything better direct access to d3 instead of using nvd3/c3 interfaces. Additionally less popular frameworks often deprecate pretty quickly. d3 seems to be stable for a really long time.

@eloyekunle Do you have any proof of concept already so we can take a look at how much of utility classes are needed?

@eloyekunle
Copy link
Contributor

@maciaszczykm No I don't have a proof of concept, but I'm working on one.

@eloyekunle
Copy link
Contributor

I have a POC up at: https://github.com/eloyekunle/d3-angular-poc.

The chart implementation is at: https://github.com/eloyekunle/d3-angular-poc/blob/master/charts/index.js, and it is used at: https://github.com/eloyekunle/d3-angular-poc/blob/master/src/app/chart/chart.component.ts#L23.

This implementation is heavily influenced by nvd3:

  • Each chart type (line/bar/pie) is implemented as a model, and configurable via chainable methods.
  • Properties can be shared across charts (e.g. width/height) and related chart types can share and inherit properties. e.g. The Donut chart type can inherit some properties from the Pie chart type.

The POC only has plain Pie and Donut charts now, without labels/legends/tooltips.

Donut Chart similar to those on Node Detail Page:
image

With a few modifications, the charts on the Node detail page can be replaced with this implementation, and we can add newer chart types as needs arise.

@floreks
Copy link
Member Author

floreks commented Mar 15, 2019

This looks really nice. I'd prefer however to use Typescript also for charts wrapper to easily include that as part of our source code (if possible). Maybe as a Factory/Service that we could use to build charts. All necessary logic could be enclosed in i.e. ChartsModule.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants