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

Switch chart engine from nvd3 to c3 #3590

Merged
merged 11 commits into from
May 30, 2019

Conversation

eloyekunle
Copy link
Contributor

@eloyekunle eloyekunle commented Feb 20, 2019

Node Detail:
image

Workload Status:
image

Closes #3571

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 20, 2019
@codecov
Copy link

codecov bot commented Feb 20, 2019

Codecov Report

Merging #3590 into master will increase coverage by 0.03%.
The diff coverage is 62.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3590      +/-   ##
==========================================
+ Coverage   46.91%   46.95%   +0.03%     
==========================================
  Files         171      171              
  Lines        7976     7972       -4     
  Branches       62       64       +2     
==========================================
+ Hits         3742     3743       +1     
+ Misses       4004     3999       -5     
  Partials      230      230
Impacted Files Coverage Δ
...end/common/components/allocationchart/component.ts 70.21% <62.85%> (+5.5%) ⬆️
...p/backend/integration/metric/common/aggregation.go 90.9% <0%> (+1.81%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ed73ca...5e196b1. Read the comment docs.

@maciaszczykm
Copy link
Member

Hey, any updates?

@eloyekunle
Copy link
Contributor Author

eloyekunle commented May 9, 2019

Hi @maciaszczykm, No updates yet, but by next week I'll be implementing the POC and the feedback from #3571 (comment) into this PR.

@eloyekunle eloyekunle changed the title WIP - Switch chart engine from nvd3 to c3 WIP - Switch chart engine from nvd3 to d3 May 9, 2019
@eloyekunle eloyekunle force-pushed the fix/d3-switch branch 2 times, most recently from fa4fa10 to bb0e44b Compare May 18, 2019 09:05
@k8s-ci-robot k8s-ci-robot added the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label May 18, 2019
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label May 19, 2019
@eloyekunle eloyekunle force-pushed the fix/d3-switch branch 3 times, most recently from 8bbd387 to 5e8270e Compare May 19, 2019 12:00
@eloyekunle eloyekunle closed this May 19, 2019
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 19, 2019
@eloyekunle eloyekunle reopened this May 19, 2019
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 19, 2019
@eloyekunle eloyekunle changed the title WIP - Switch chart engine from nvd3 to d3 WIP - Switch chart engine from nvd3 to c3 May 19, 2019
@eloyekunle eloyekunle changed the title WIP - Switch chart engine from nvd3 to c3 Switch chart engine from nvd3 to c3 May 19, 2019
this.generateGraph_();
allocated = new Set();

ngAfterViewInit(): void {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, why can't we use ngOnInit() here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ajatprabha It wasn't displaying the chart with ngOnInit().

@maciaszczykm
Copy link
Member

@eloyekunle Looks promising! Can you solve the conflicts?

@floreks
Copy link
Member

floreks commented May 30, 2019

@eloyekunle PR lgtm. Rebase please and we can merge.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 30, 2019
@maciaszczykm
Copy link
Member

You have to commit package-lock.json as you have also done some changes to the package.json. CI is failing due to that.

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 30, 2019
@eloyekunle
Copy link
Contributor Author

@maciaszczykm Fixed now.

@maciaszczykm
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 30, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: eloyekunle, maciaszczykm

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 30, 2019
@k8s-ci-robot k8s-ci-robot merged commit d222ace into kubernetes:master May 30, 2019
@eloyekunle eloyekunle deleted the fix/d3-switch branch May 31, 2019 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use c3 instead of nvd3
5 participants