-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Set the maximum width of categorical bars & boxes to category unit - case of categoryarray #5732
Conversation
Behaviour looks good. |
Interestingly, box and violin don't seem to suffer from this problem even on master ... normal? |
Positive. Thanks for checking. |
src/traces/bar/sieve.js
Outdated
|
||
this.distinctPositions = dv.vals; | ||
if(dv.vals.length === 1 && width1 !== Infinity) this.minDiff = width1; | ||
else this.minDiff = Math.min(dv.minDiff, width1); | ||
|
||
var type = (opts.posAxis || {}).type; | ||
if(type === 'category' || type === 'multicategory') { | ||
this.minDiff = Math.min(1, this.minDiff); |
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.
This looks like a great solution, and simpler than the previous logic. My only question is whether there's actually a benefit of doing Math.min
vs just minDiff = 1
in the (multi)category case? There's no way to specify input data that will result in non-integer positions here, is there?
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.
Good call. Set to one in 6e6bfa6.
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.
💃 after considering https://github.com/plotly/plotly.js/pull/5732/files#r653694361
Co-authored-by: Alex Johnson <johnson.alex.c@gmail.com>
Fixes #5731.
@plotly/plotly_js