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

cast getRightValue to number #5947

Merged
merged 1 commit into from Jan 2, 2019
Merged

cast getRightValue to number #5947

merged 1 commit into from Jan 2, 2019

Conversation

kurkle
Copy link
Member

@kurkle kurkle commented Jan 1, 2019

1 + '0' evaluates to '10', so stack calculation does not work correctly if values are strings.

This was changed in #4565 where also linear scale was made to cast string to number.
However logarithmic scale uses implementation from Scale where that casting is not done (and can't be since its used for category scale as well)

Wrong size calculation is fixed also per comment by @nagix in original PR.
This calculated size (or center) are currently not used for anything, so that part does not affect any tests.

Fixes: #5862
Replaces: #5892
https://codepen.io/kurkle/pen/KrjXxL

Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

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

My only comment is that if we ever document the scale API, we should try and specify that getRightValue returns a number so that we don't need to add in extras to do the conversion

@benmccann
Copy link
Contributor

Would changing scale.getRightValue now be a breaking change? I agree that'd be the nicer way to do it

@simonbrunel
Copy link
Member

simonbrunel commented Jan 1, 2019

we should try and specify that getRightValue returns a number

Are we sure it will be always the case? can't we have string values displayed on a category scale? And what about date / time object data for time scale? The return type of getRightValue() depends of the scale type, so this can't be number for all scales. So #5949 doesn't make sense IMO.

Something we should consider in v3 is to move the stacking logic in the scale instead to have each controller to implement a generic method to stack values that works only with numbers.

@simonbrunel simonbrunel added this to the Version 2.8 milestone Jan 1, 2019
@simonbrunel simonbrunel merged commit 918e2c0 into chartjs:master Jan 2, 2019
@simonbrunel
Copy link
Member

Thanks @kurkle

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

Successfully merging this pull request may close these issues.

[BUG] 0 value in stacked grouped bar overshoots with logarithmic.
4 participants