Repository navigation
Fixing BarSeries overlapped - #944
Conversation
mcnuttandrew
left a comment
There was a problem hiding this comment.
I like what you have going here, but the algebra of these statements is really hard to read. Can you include some notes explaining why these changes are necessary in the code?
| 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! |
mcnuttandrew
left a comment
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 😳 |
|
|
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