Skip to content

fix: stacked bar chart minBarLength causes overlapping - #10766

Merged
etimberg merged 2 commits into
chartjs:masterfrom
Kit-p:fix/stacked-bar-minBarLength
Nov 23, 2022
Merged

fix: stacked bar chart minBarLength causes overlapping#10766
etimberg merged 2 commits into
chartjs:masterfrom
Kit-p:fix/stacked-bar-minBarLength

Conversation

@Kit-p

@Kit-p Kit-p commented Oct 7, 2022

Copy link
Copy Markdown
Contributor

This PR introduces a new private field _visualValues in meta.parsed._stacks[axis], which stores the data coordinates of the bar rendered after applying minBarLength, and is then used in applyStack() instead of the actual value.

Added two fixture unit tests for the expected behavior of both vertical and horizontal stacked bar chart, using linear axis.

Please note that this PR did not handle logarithmic axis properly. Would appreciate if anyone can help on that.

Thank you @etimberg and @kurkle for all the help on Slack! Feedback is welcome!

Closes #10664.

@nmakhari

Copy link
Copy Markdown

Hi, I'm having an issue with floating, stacked bars (#10774). I was wondering if you could point me in the right direction in terms of what I can do to get a fix for this. I have zero familiarity with the code base, so digging around blindly isn't doing anything for me.

It seems you have worked in this area, and any help you can provide would be greatly appreciated!

@kurkle kurkle left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

While this seems to be a working and quite clean solution, I don't really like it.
Other datasets also now read the _visualValues, which are just a copy of what was previously read. Are the other values used for something (in bar controller) now?

@kurkle
kurkle requested a review from etimberg October 17, 2022 18:44
@Kit-p

Kit-p commented Oct 18, 2022

Copy link
Copy Markdown
Contributor Author

@kurkle Yes, I also agree that there should be a more technically correct way of doing this. Unfortunately, I am not familiar with the codebase enough to figure that out.

Regarding what is _visualValues, I do not understand your classification of it being just a copy of what was previously read. It does store a calculated result that seems to be lost in the next iteration. I interpret it as a bridge to bring the effect of minBarLength into the applyStack() function, which otherwise needs to do a O(n^2) computation to find the correct value, or we will have to do some complex calculations (the technically correct way) in the controller function.

Finally, which other values are you referring to? If you mean _visualValues, then no, they do not serve for any other purposes.

@kurkle kurkle left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

me not liking this should not be a blocker. also its all internals, so it can be adjusted in upcoming releases if need be.

@etimberg

Copy link
Copy Markdown
Member

I guess we can try this. The implementation does give me pause for the reasons @kurkle mentioned. We can always adjust or remove later if it's a problem

@etimberg
etimberg merged commit 667b28b into chartjs:master Nov 23, 2022
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.

minBarLength Stacked bar chart not working

5 participants