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

pass ratio to the data_labels_format callback when appropriate #2694

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

panthony
Copy link
Contributor

That one is a breaking change.

The data_labels_format callback now takes the ratio alongs with value and index.

The ratio is undefined if the chart is not normalized.

I also passed $$.api as this for the format callback in case it can be useful.

@codecov-io
Copy link

Codecov Report

Merging #2694 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2694      +/-   ##
==========================================
+ Coverage   81.02%   81.03%   +<.01%     
==========================================
  Files          59       59              
  Lines        4633     4634       +1     
==========================================
+ Hits         3754     3755       +1     
  Misses        879      879
Impacted Files Coverage Δ
src/text.js 98.7% <100%> (+0.01%) ⬆️

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 4163be5...0b65108. Read the comment docs.

1 similar comment
@codecov-io
Copy link

codecov-io commented Aug 20, 2019

Codecov Report

Merging #2694 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2694      +/-   ##
==========================================
+ Coverage   81.02%   81.03%   +<.01%     
==========================================
  Files          59       59              
  Lines        4633     4634       +1     
==========================================
+ Hits         3754     3755       +1     
  Misses        879      879
Impacted Files Coverage Δ
src/text.js 98.7% <100%> (+0.01%) ⬆️

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 4163be5...0b65108. Read the comment docs.

@kt3k
Copy link
Member

kt3k commented Aug 25, 2019

@panthony Isn't it possible to make this not a breaking change? How about making v have the shape of { value: number, ratio: number } only when stack.normalize == true, or passing ratio as 5th argument?

@panthony
Copy link
Contributor Author

@kt3k I can pass it as 5th argument but it would mean that we'll have callbacks that takes:

(value, ratio, id)

And others:

(value, id, i, j, ratio)

Passing an object would still be a breaking change

@kt3k
Copy link
Member

kt3k commented Aug 25, 2019

@panthony

we'll have callbacks that takes:
(value, ratio, id)
And others:
(value, id, i, j, ratio)

Ah, that's bad...

Another option could be introducing new option data_labels_formatter (or something) which takes 5 arguments and keep data_labels_format take 4 arguments, though it looks a little tricky to implement and document...

@kt3k
Copy link
Member

kt3k commented Aug 25, 2019

Anyway we haven't introduced such explicit breaking changes recently except d3 v5 update, I'm going to talk with @masayuki0812 about the policy and how we update the version number if we do breaking changes.

@panthony
Copy link
Contributor Author

@kt3k Not sure if always possible but maybe we could pass directly the data point in all callbacks.

This way we have everything (id, index, value, ratio, any other attributes that may be added) and new attributes could be added in the future without API break.

To ease the upgrade we could introduce a boolean in the config to toggle the previous/new behavior.

Config that would eventually be dropped.

@kt3k
Copy link
Member

kt3k commented Aug 25, 2019

@panthony

This way we have everything (id, index, value, ratio, any other attributes that may be added) and new attributes could be added in the future without API break.

Sounds nice 👍 I prefer to switch to that API in some future.

To ease the upgrade we could introduce a boolean in the config to toggle the previous/new behavior.

That's sounds reasonable. It would make the upgrade flow more graceful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants