Fixing BarSeries overlapped #944
Conversation
src/plot/series/bar-series-canvas.js
Outdated
| const valuePos = Math.min(value0(row), value(row)); | ||
| const x = valuePosAttr === 'x' ? valuePos : linePos; | ||
| const y = valuePosAttr === 'y' ? valuePos : linePos; | ||
|
|
||
| const lineSize = itemSize * 2 / sameTypeTotal; | ||
| const lineSize = ((itemSize * 2 / sameTypeTotal) - (1 - (1 / sameTypeTotal))); |
There was a problem hiding this comment.
Maybe clearer as (itemSize * 2 - (sameTypeTotal - 1)) / sameTypeTotal ?
|
@mcnuttandrew Sure, I'll add some comments. I think it would also help to add some variables for intermediate values (with appropriate descriptive names). Do you think that would be ok? |
|
@gsas2 that sounds like a great idea, value ease of reading here! |
There was a problem hiding this comment.
Wow that's a lot of text! Sorry to give you the run around here, but would you mind reducing each of those comments to around 2-4 lines? Readability and brevity are closely related. I think your variable names do more work than you are expected of them at the moment
|
Sure. Sorry I didn't know where to stop 😳 |
|
|
gsas2
force-pushed
the
fix/barSeriesOverlapped
branch
from
996a5da to
0498293
Compare
|
@mcnuttandrew I'd been trying to fix it by reducing the space (instead of adding 1 pixel, using some smaller values like 0.90), but while that might fix this new issue, it makes the original overlapping to appear back (but with less overlapping). |
|
Hi @mcnuttandrew, do you have any comments/thoughts around this? |
|
This seems fine? If you'll clean up the merge conflict i'll merge |
|
@mcnuttandrew Merge conflicts solved. Please, let me know if anything else is missing. Thanks! |
|
@gsas2 sorry about the delay i've been traveling. Thanks again for addressing this! |
This PR adjusts the calculation of bar width and starting position, to avoid bars "of same type" getting overlapped.
Reference: #936
Before
After