feat: avoid label collision #247
Conversation
MAudelGisaia
requested review from
sebbousquet,
mbarbet,
MohamedHamouGisaia and
QuCMGisaia
| @@ -70,6 +70,7 @@ export class HistogramParams { | |||
| public ticksDateFormat: string = null; | |||
| public xAxisPosition: Position = Position.bottom; | |||
| public descriptionPosition: Position = Position.bottom; | |||
| public overlapXTolerance = 2; | |||
There was a problem hiding this comment.
could be more if needed, it apply a padding of 2 on left and right side, to detect the overlap before the label collide.
There was a problem hiding this comment.
Maybe it should be an input ?
There was a problem hiding this comment.
could be a good idea !
There was a problem hiding this comment.
Yes good Idea, we should add a new input for this overlap tolerance
src/histograms/AbstractHistogram.ts
Outdated
| } | ||
| } | ||
|
|
||
| if(!this._xlabelMeanWidth) { |
There was a problem hiding this comment.
Why not recompute it each time ?
There was a problem hiding this comment.
this var helps me to know when i can put more label when the chart grow. So i cacl the mean size once, when all label are displayed.
There was a problem hiding this comment.
The virtual labels always has the maximum number of labels right ?
| for (let i = 0; i < labels.size(); i++) { | ||
| const next = i + 1; | ||
| if(nodes[next]){ | ||
| const c = this.getDimension(nodes[i]); |
There was a problem hiding this comment.
Do 'c' and 'n' stand for current and next ? if so, can you add a comment for those var names or rename the variable ?
| const horizontalOffset = this.getHorizontalOffset(chartAxes); | ||
| let sumWidth = 0; | ||
| // create virtual nodes. Helps to get label's width. | ||
| const virtualLabels = this.chartDimensions.svg.append('g'); |
There was a problem hiding this comment.
I think you can try to select the already existing labels.
Or you need to use one common method between the real label axis and the virtual one in order to avoid duplication
| @@ -70,6 +70,7 @@ export class HistogramParams { | |||
| public ticksDateFormat: string = null; | |||
| public xAxisPosition: Position = Position.bottom; | |||
| public descriptionPosition: Position = Position.bottom; | |||
| public overlapXTolerance = 2; | |||
There was a problem hiding this comment.
Yes good Idea, we should add a new input for this overlap tolerance
|
LGTM ! |
Capture.video.du.03-02-2025.18.25.25.webm