Feature/improved point label position (redux of #2045) #2047
Conversation
There was a problem hiding this comment.
Thanks for the PR!
I really like the approach taken here and I think it looks good with bubble charts. The spacing between the text and bubble edge in this mode is made very consistent and looks great!
Old:
New:
However, subjectively I find this 45deg angle offset less readable generally than the x-offset version in the old charts, especially without bubbling the next can get lost when the scatter area is crowded.
Old:
New:
I propose we keep the radius calculation for the x-offset, but remove the 45deg angle (e.g. LABEL_COSINE is 1 and no y-offset is applied). This will get us consistent spacing on the x-axis without the (IMO) confusing label placement when data is clustered.
I've done this change in #2067, which I've also rebased onto master and removed the "cargo.lock" commits. Please prefer rebase to git merge master and be sure to squash commits for open Perspective PRs - for a project of this size and regression coverage, it is of great help to have as clean a change history as possible.
| @@ -12,6 +12,9 @@ import { groupFromKey } from "./seriesKey"; | |||
| import { fromDomain } from "./seriesSymbols"; | |||
| import { toValue } from "../tooltip/selectionData"; | |||
|
|
|||
| const LABEL_PADDING = 5; | |||
There was a problem hiding this comment.
Global const is better than magic numbers but we can also read these values from CSS variables if we want - these are cached when the plugin is first rendered so the stylesheet is not queried on every render. See plugin.js for examples.
|
|
||
| context.fillText(value, offset, 4); | ||
| let magnitude = 10; |
There was a problem hiding this comment.
... and we should do them all :)
| // r = sqrt(A / pi) | ||
| const radius = Math.sqrt( | ||
| (scale_factor * size(d.size)) / Math.PI | ||
| ); |
There was a problem hiding this comment.
This was news to me - I had assumed that the .size() method meant radius, but according to the docs you are indeed correct, this is actually area!
Currently tooltips (labels) of point data have the possibility of overlapping the actual point area. This PR is an attempt to provide a more calculated offset based on the area of the point.
redux of this PR