Contour label extra pad and correct minus sign #4540
Conversation
... to get some formatting additions like the correct minus sign
... that is add 2 px around labels text clip out of contour lines below.
This becomes especially useful when for negative contour labels,
to differentiate between the minus sign and the contour line
src/traces/contour/constants.js
Outdated
| }, | ||
|
|
||
| // number of px around labels text clip out of contour lines below | ||
| LABEL_LINE_CLIP_PAD: 2 |
There was a problem hiding this comment.
Could you please try 3px horizontal padding here?
There was a problem hiding this comment.
2 px looks like plenty to me, but I might argue for making it a fraction of the font size (or text height) so that it scales with the text.
src/traces/contour/plot.js
Outdated
| var halfWidth = textOpts.width / 2; | ||
| var halfHeight = textOpts.height / 2; | ||
| var halfWidth = textOpts.width / 2 + LABEL_LINE_CLIP_PAD; | ||
| var halfHeight = textOpts.height / 2 + LABEL_LINE_CLIP_PAD; |
There was a problem hiding this comment.
After viewing the baseline changes, I think we may not need to add vertical padding to the height.
There was a problem hiding this comment.
I agree, let's not add vertical padding. In fact if there was a way to make it even a tighter fit vertically than it currently is on master, I'd be all for it. Probably not though, assuming that comes from a bounding box... but the goal is to avoid clipping neighboring contour lines as much as possible, since that can cause confusion about which line is actually being labeled.
|
Thanks for adding commits @archmoj and taking over this PR !! Looks like your solution uses a constant padding and does not attempt to implement @alexcjohnson's
idea, so I'll let @alexcjohnson confirm that the updated baselines look 👌 |
src/traces/contour/plot.js
Outdated
| var halfWidth = textOpts.width / 2; | ||
| var halfHeight = textOpts.height / 2; | ||
| var w = textOpts.width + 4; | ||
| var h = Math.max(0, textOpts.height - 4); |
There was a problem hiding this comment.
This looks great for all the baselines we test. But I'm still concerned about it in case someone uses a very different font size, particularly shrinking height by a constant if someone were to use a very small font size, perhaps for publication when it's expected the user can zoom in on the PDF or something.
So yeah, I'd still like to make both the +4 and -4 adjustments relative to font size, or if you think there's some ill effect we'd expect from that, at least ensure the -4 isn't too large a fraction of the height.
|
@alexcjohnson this PR should be ready for another review. |
my attempt to fix #4415
Best to review this PR by comparing the baseline changes in each commit. The changes induces in the first commit a32f76f should be uncontroversial. I'm not 100% sure about the results in commit 30bee29 though: most baselines look at lot better to me, but some appear to have a little too much padding e.g.
cheater_contour,contour_label-formatting-via-colorbar,contour_lines_coloring,heatmap_multicategory.@plotly/plotly_js please let me know what you think.