fix: marker positioning #1538
fix: marker positioning #1538
Conversation
|
We should also have a look at the e2e graphics test cases. I think there are more tests failing than expected such as the tests which have markers positioned above, and below. |
| @@ -131,14 +131,16 @@ export class SeriesMarkersPaneView implements IUpdatablePaneView { | |||
| } | |||
|
|
|||
| public autoScaleMargins(): AutoScaleMargins | null { | |||
| if (this._autoScaleMarginsInvalidated) { | |||
| if (this._autoScaleMarginsInvalidated && this._dataInvalidated) { | |||
There was a problem hiding this comment.
This condition is always false if this._makeValid() is called first
There was a problem hiding this comment.
And margins depend not only on data but also on bar spacing
There was a problem hiding this comment.
Apologies, my mistake for mentioning to try run the calculation less often at that point.
We can instead limit the _dataInvalidated check to only the _hasAllMarkerSamePosition() method because that doesn't rely on barSpacing.
There was a problem hiding this comment.
Yes we can, but we need to add one additional flag like _hasAllMarkerSamePositionInvalidated and use it in _hasAllMarkerSamePosition or update cached value in _makeValid
Type of PR: bugfix, enhancement
PR checklist:
Overview of change:
Fixed an issue where
marginBelowwasn't applied. Added an enhancement that fixes #1382: When a series has only one type of markers, we apply only one margin, i.e. for markers withposition: "aboveBar"we remove bottom margin and vice versa.Is there anything you'd like reviewers to focus on?