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 6 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 |
| // Dynamic presentation safety buffer to clear tilted axis tick labels | ||
| var axisPad = 0; | ||
| if (!isHorizontal && opts.xa && opts.xa.side === 'top') { | ||
| if (opts.xa._g && opts.xa._g.node()) { | ||
| var axisBB = opts.xa._g.node().getBBox(); | ||
| if (axisBB && axisBB.height > 0) { | ||
| // Shift exactly past the bounding height of the tilted labels plus a clean 6px visual gap | ||
| axisPad = axisBB.height + 6; | ||
| } | ||
| } | ||
| } else if (isHorizontal && opts.ya && opts.ya.side === 'right') { | ||
| if (opts.ya._g && opts.ya._g.node()) { | ||
| var axisBB = opts.ya._g.node().getBBox(); | ||
| if (axisBB && axisBB.width > 0) { | ||
| // Shift exactly past the bounding width of the side labels plus a clean 6px visual gap | ||
| axisPad = axisBB.width + 6; | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I believe this can all be removed now that the zeroBarDir logic has been added (please correct me if I'm missing something!). I tested it out locally and the issue seems resolved with just the zeroBarDir changes.
Along with removing the references to axisPad below
| // Dynamic presentation safety buffer to clear tilted axis tick labels | |
| var axisPad = 0; | |
| if (!isHorizontal && opts.xa && opts.xa.side === 'top') { | |
| if (opts.xa._g && opts.xa._g.node()) { | |
| var axisBB = opts.xa._g.node().getBBox(); | |
| if (axisBB && axisBB.height > 0) { | |
| // Shift exactly past the bounding height of the tilted labels plus a clean 6px visual gap | |
| axisPad = axisBB.height + 6; | |
| } | |
| } | |
| } else if (isHorizontal && opts.ya && opts.ya.side === 'right') { | |
| if (opts.ya._g && opts.ya._g.node()) { | |
| var axisBB = opts.ya._g.node().getBBox(); | |
| if (axisBB && axisBB.width > 0) { | |
| // Shift exactly past the bounding width of the side labels plus a clean 6px visual gap | |
| axisPad = axisBB.width + 6; | |
| } | |
| } | |
| } |
| 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; |
| var dir = isHorizontal ? dirSign(x1, x0) : dirSign(y0, y1); | ||
| if ((isHorizontal ? x0 === x1 : y0 === y1) && opts.zeroBarDir) { | ||
| dir = opts.zeroBarDir; | ||
| } |
There was a problem hiding this comment.
This is logically equivalent, just a style suggestion
| var dir = isHorizontal ? dirSign(x1, x0) : dirSign(y0, y1); | |
| if ((isHorizontal ? x0 === x1 : y0 === y1) && opts.zeroBarDir) { | |
| dir = opts.zeroBarDir; | |
| } | |
| var dir; | |
| if ((isHorizontal ? x0 === x1 : y0 === y1) && opts.zeroBarDir) { | |
| dir = opts.zeroBarDir; | |
| } else { | |
| dir = isHorizontal ? dirSign(x1, x0) : dirSign(y0, y1); | |
| } |
| plot: plot, | ||
| toMoveInsideBar: toMoveInsideBar | ||
| toMoveInsideBar: toMoveInsideBar, | ||
| toMoveOutsideBar: toMoveOutsideBar |
There was a problem hiding this comment.
I believe this can also be removed
| toMoveOutsideBar: toMoveOutsideBar |
| .then(done, done.fail); | ||
| }); | ||
|
|
||
| it('should keep zero-value outside labels on the same side as negative bars', function(done) { |
| angle: angle, | ||
| xa: xa, // Pass the X-Axis configuration | ||
| ya: ya, // Pass the Y-Axis configuration |
There was a problem hiding this comment.
I think these are also no longer needed
| angle: angle, | |
| xa: xa, // Pass the X-Axis configuration | |
| ya: ya, // Pass the Y-Axis configuration |
| @@ -0,0 +1 @@ | |||
| - Prevent outside bar text labels from overlapping tilted axis ticks [[#7872](https://github.com/plotly/plotly.js/pull/7872)] | |||
There was a problem hiding this comment.
| - Prevent outside bar text labels from overlapping tilted axis ticks [[#7872](https://github.com/plotly/plotly.js/pull/7872)] | |
| - Prevent outside bar text for zero-length bars from overlapping axis tick labels [[#7872](https://github.com/plotly/plotly.js/pull/7872)] |
3 participants
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.