fix(bar): prevent outside text labels from overlapping tilted axis ticks - #7872
fix(bar): prevent outside text labels from overlapping tilted axis ticks#7872vizansh wants to merge 9 commits into
Conversation
|
Thanks for the PR! Our team will review and provide feedback. |
|
Hi maintainers, could someone please approve and trigger the GitHub Actions workflow for this PR? I'd love to make sure all the automated CI/CD tests pass successfully. Thank you! |
|
Until we get to this review, could you please look into the test failures and see if anything is broken/needs to be updated? |
|
Hi @vizansh ! I just spent some time reviewing the original issue #7822, and posted a comment with my thoughts on the root cause and the best approach for a fix. I think the best fix will require a different approach than what you've done here. Please feel free to continue working and pursue one of the fix approaches outlined in my comment if you like, or propose another approach. I'm happy to review. |
|
/review |
Co-authored-by: Emily KL <4672118+emilykl@users.noreply.github.com>
Co-authored-by: Emily KL <4672118+emilykl@users.noreply.github.com>
|
Hi @emilykl! Thank you so much for the detailed review and guidance! Those remaining inline errors were just leftover artifacts from my very first commit attempt. All of your suggested refactors and cleanups have been applied in the latest commits. Ready for another review whenever you have a moment! Thanks again for all your help! |
| targetX = x1 - dir * (textpad + axisPad); | ||
| anchorX = dir * extrapad; | ||
| } else { | ||
| targetY = y1 + dir * textpad; | ||
| targetY = y1 + dir * (textpad + axisPad); |
There was a problem hiding this comment.
| targetX = x1 - dir * (textpad + axisPad); | |
| anchorX = dir * extrapad; | |
| } else { | |
| targetY = y1 + dir * textpad; | |
| targetY = y1 + dir * (textpad + axisPad); | |
| targetX = x1 - dir * textpad; | |
| anchorX = dir * extrapad; | |
| } else { | |
| targetY = y1 + dir * textpad; |
There was a problem hiding this comment.
@vizansh I've added a new comment with the suggestion, could you apply it? The reference to axisPad should be removed.
| isHorizontal: isHorizontal, | ||
| constrained: constrained, | ||
| angle: angle | ||
| angle: angle, |
There was a problem hiding this comment.
| angle: angle, | |
| angle: angle, | |
| zeroBarDir: zeroBarDir |
angle should be added back in here. My fault, I mistakenly suggested deleting it.
| isHorizontal: isHorizontal, | ||
| constrained: constrained, | ||
| angle: angle | ||
| zeroBarDir: zeroBarDir |
There was a problem hiding this comment.
| zeroBarDir: zeroBarDir | |
| angle: angle, | |
| zeroBarDir: zeroBarDir |
angle should be added back in here. My fault, I mistakenly suggested deleting it.
|
@vizansh A couple more cleanup suggestions to apply, then this should be good. I tried applying them myself but got an error so hopefully it works on your side. Thanks for the quick response! |
Fixes #7822
This pull request dynamically calculates the positioning for bar chart text labels when
textposition='outside'.Instead of using a static pixel margin, it looks up the active axis configuration and utilizes
.getBBox()to measure the exact live dimensions of the tilted or long axis labels. It then shifts the bar numbers by that precise footprint plus a clean visual buffer, entirely preventing overlaps with the top/right axis ticks.