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

The path for background arc needs to be supplied with data to get it rendered. This went missing. #2401

Merged
merged 1 commit into from Jul 6, 2018

Conversation

Projects
None yet
5 participants
@rahul-winner
Copy link
Contributor

rahul-winner commented Jul 5, 2018

This is for fixing the issue #2390

Solution : The path for background arc needs to be supplied with data to get it rendered.

Singhai
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jul 5, 2018

Codecov Report

Merging #2401 into master will increase coverage by 0.09%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2401      +/-   ##
==========================================
+ Coverage   78.05%   78.14%   +0.09%     
==========================================
  Files          52       52              
  Lines        4137     4136       -1     
==========================================
+ Hits         3229     3232       +3     
+ Misses        908      904       -4
Impacted Files Coverage Δ
src/arc.js 73.95% <100%> (+1.29%) ⬆️

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 f48c217...f4e1fb1. Read the comment docs.

@rahul-winner

This comment has been minimized.

Copy link
Contributor Author

rahul-winner commented Jul 5, 2018

@kt3k
did we take any decision for merging it to master? I believe you are the correct person who can provide some information.
Pls let me know if you are seeing any functional deviation or issue here.

Thanks a lot !

@panthony

This comment has been minimized.

Copy link
Contributor

panthony commented Jul 5, 2018

This PR may also fix #2343

@kt3k

This comment has been minimized.

Copy link
Member

kt3k commented Jul 6, 2018

@rahul-winner
I confirmed this change fixes the example in #2390 (and in #2343). Thank you for the contribution!

Do you think we can add any test case to prevent this regression?

@kt3k kt3k merged commit a47d066 into c3js:master Jul 6, 2018

2 checks passed

ci/circleci: test Your tests passed on CircleCI!
Details
codecov/project 78.14% (+0.09%) compared to f48c217
Details
@kt3k

This comment has been minimized.

Copy link
Member

kt3k commented Jul 6, 2018

Released as 0.6.3!

@rahul-winner rahul-winner deleted the rahul-winner:fix_2390 branch Jul 6, 2018

@rahul-winner

This comment has been minimized.

Copy link
Contributor Author

rahul-winner commented Jul 6, 2018

@kt3k
I'll explore if we could have test case for ensuring this behavior going further and will create PR.

Thanks a lot for expediting this fix !

@1Jesper1

This comment has been minimized.

Copy link

1Jesper1 commented Jul 12, 2018

@rahul-winner

https://c3js.org/samples/chart_gauge.html

Add this code to the chart data:

setTimeout(function () {
    chart.resize({height:100, width:300})
}, 1000);

setTimeout(function () {
    chart.resize({height:200})
}, 2000);

setTimeout(function () {
    chart.resize();
}, 3000);

Background arc does not resize.

@rahul-winner

This comment has been minimized.

Copy link
Contributor Author

rahul-winner commented Jul 12, 2018

@1Jesper1 thanks for your comment.

Apparently some more work be required on background arc after moving on to recent D3. I'm investigating it and will try to fix it. Please feel free to contribute if you are seeing any fix.

@rahul-winner

This comment has been minimized.

Copy link
Contributor Author

rahul-winner commented Jul 13, 2018

let's track the resize issue here #2409

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment