Navigation Menu

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

fix: stacked bar chart minBarLength causes overlapping #10766

Merged
merged 2 commits into from Nov 23, 2022

Conversation

Kit-p
Copy link
Contributor

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

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

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!

Copy link
Member

@kurkle kurkle left a comment

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
Copy link
Contributor Author

Kit-p commented Oct 18, 2022

@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.

Copy link
Member

@kurkle kurkle left a comment

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
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