-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
add legend format #2232
base: master
Are you sure you want to change the base?
add legend format #2232
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2232 +/- ##
=======================================
Coverage 73.88% 73.88%
=======================================
Files 51 51
Lines 4185 4185
=======================================
Hits 3092 3092
Misses 1093 1093 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution.
Please address the requested changes so that we can proceed.
c3.js
Outdated
typeof exports === 'object' && typeof module !== 'undefined' ? module.exports = factory() : | ||
typeof define === 'function' && define.amd ? define(factory) : | ||
(global.c3 = factory()); | ||
typeof exports === 'object' && typeof module !== 'undefined' ? module.exports = factory() : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You modified the distribution file!
You should modify the proper file from src directory and not commit the c3.js file which is overwritten during release
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is just the indentation issue with my editor. I don't see any chages here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I meant is that you modified the c3.js
file which is the distribution file and that this file should not be part of the pull request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@panthony i have make the requested changes. I have moved the logic to legend.js and config.js
You can review chnages.
c3.js
Outdated
@@ -5097,6 +5097,7 @@ c3_chart_internal_fn.getDefaultConfig = function () { | |||
legend_padding: 0, | |||
legend_item_tile_width: 10, | |||
legend_item_tile_height: 10, | |||
legend_format: undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, also add the documentation for this new option. The documentation site has been added to the repository.
c3.js
Outdated
if(config.legend_format && isFunction(config.legend_format)) { | ||
return config.legend_format(id, index); | ||
} else { | ||
return isDefined(config.data_names[id]) ? config.data_names[id] : id; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, do not repeat the logic that resolve the legend's name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't get you here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pkpatels I saw the same logic twice but it must have been between c3.js and legend.js files.
Your pull request still includes the c3.js
file although it should not be committed.
@panthony I have made the requested changes. |
No description provided.