-
Notifications
You must be signed in to change notification settings - Fork 2.5k
feat(series): add connectGaps option to line, area and baseline series #2075
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -231,6 +231,13 @@ export interface LineStyleOptions { | |||||
| * @defaultValue {@link LastPriceAnimationMode.Disabled} | ||||||
| */ | ||||||
| lastPriceAnimation: LastPriceAnimationMode; | ||||||
|
|
||||||
| /** | ||||||
| * Connect gaps in data. | ||||||
| * | ||||||
| * @defaultValue `true` | ||||||
| */ | ||||||
| connectGaps?: boolean; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Every other property in this interface (e.g.
Suggested change
The JSDoc Same change needed in |
||||||
| } | ||||||
|
|
||||||
| /** | ||||||
|
|
@@ -351,6 +358,13 @@ export interface AreaStyleOptions { | |||||
| * @defaultValue {@link LastPriceAnimationMode.Disabled} | ||||||
| */ | ||||||
| lastPriceAnimation: LastPriceAnimationMode; | ||||||
|
|
||||||
| /** | ||||||
| * Connect gaps in data. | ||||||
| * | ||||||
| * @defaultValue `true` | ||||||
| */ | ||||||
| connectGaps?: boolean; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
|
|
@@ -504,6 +518,13 @@ export interface BaselineStyleOptions { | |||||
| * @defaultValue {@link LastPriceAnimationMode.Disabled} | ||||||
| */ | ||||||
| lastPriceAnimation: LastPriceAnimationMode; | ||||||
|
|
||||||
| /** | ||||||
| * Connect gaps in data. | ||||||
| * | ||||||
| * @defaultValue `true` | ||||||
| */ | ||||||
| connectGaps?: boolean; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,6 +25,7 @@ export class SeriesLinePaneView extends LinePaneViewBase<'Line', LineStrokeItem, | |
| pointMarkersRadius: options.pointMarkersVisible ? (options.pointMarkersRadius || options.lineWidth / 2 + 2) : undefined, | ||
| visibleRange: this._itemsVisibleRange, | ||
| barWidth: this._model.timeScale().barSpacing(), | ||
| connectGaps: options.connectGaps as boolean, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Same applies to the casts in |
||
| }; | ||
|
|
||
| this._renderer.setData(data); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,7 +1,7 @@ | ||||||
| import { BitmapCoordinatesRenderingScope } from 'fancy-canvas'; | ||||||
|
|
||||||
| import { Coordinate } from '../model/coordinate'; | ||||||
| import { SeriesItemsIndexesRange } from '../model/time-data'; | ||||||
| import { SeriesItemsIndexesRange, TimePointIndex } from '../model/time-data'; | ||||||
|
|
||||||
| import { LinePoint, LineType } from './draw-line'; | ||||||
|
|
||||||
|
|
@@ -10,7 +10,7 @@ function distanceByCoordinates(p1x: number, p1y: number, p2x: number, p2y: numbe | |||||
| } | ||||||
|
|
||||||
| // eslint-disable-next-line max-params, complexity | ||||||
| export function walkLine<TItem extends LinePoint, TStyle extends CanvasRenderingContext2D['fillStyle' | 'strokeStyle']>( | ||||||
| export function walkLine<TItem extends LinePoint & { time?: TimePointIndex }, TStyle extends CanvasRenderingContext2D['fillStyle' | 'strokeStyle']>( | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
Suggested change
With |
||||||
| renderingScope: BitmapCoordinatesRenderingScope, | ||||||
| items: readonly TItem[], | ||||||
| lineType: LineType, | ||||||
|
|
@@ -20,7 +20,8 @@ export function walkLine<TItem extends LinePoint, TStyle extends CanvasRendering | |||||
| // so if styleGetter returns objects, then styleGetter should return the same object for equal styles | ||||||
| styleGetter: (renderingScope: BitmapCoordinatesRenderingScope, item: TItem) => TStyle, | ||||||
| finishStyledArea: (renderingScope: BitmapCoordinatesRenderingScope, style: TStyle, areaFirstItem: LinePoint, newAreaFirstItem: LinePoint) => void, | ||||||
| dashPatternLength: number = 0 | ||||||
| dashPatternLength: number = 0, | ||||||
| connectGaps: boolean = true | ||||||
| ): void { | ||||||
| if (items.length === 0 || visibleRange.from >= items.length || visibleRange.to <= 0) { | ||||||
| return; | ||||||
|
|
@@ -74,19 +75,29 @@ export function walkLine<TItem extends LinePoint, TStyle extends CanvasRendering | |||||
| const currentY = currentItem.y * verticalPixelRatio; | ||||||
| const itemStyle = styleGetter(renderingScope, currentItem); | ||||||
|
|
||||||
| const prevItem = items[i - 1]; | ||||||
| const isGap = !connectGaps && currentItem.time !== undefined && prevItem.time !== undefined && currentItem.time - prevItem.time > 1; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Two issues with this gap detection:
The intended behaviour (per discussion) is: only break the line where this series has explicit whitespace data between two points. If the user wants gaps for timestamps that only exist in another series, they should add whitespace entries to this series. This means the gap detection can't be done purely at the renderer level by comparing One approach: during item construction in the pane view (e.g. in |
||||||
|
|
||||||
| if (isGap) { | ||||||
| finishStyledArea(renderingScope, currentStyle, currentStyleFirstItem, prevItem); | ||||||
| ctx.beginPath(); | ||||||
| currentStyle = itemStyle; | ||||||
| currentStyleFirstItem = currentItem; | ||||||
| ctx.moveTo(currentX, currentY); | ||||||
| continue; | ||||||
| } | ||||||
|
Comment on lines
+81
to
+88
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: When the line breaks at a gap, if (shouldTrackDashOffset) {
const offset = accumulatedDistance % dashPatternLength;
ctx.lineDashOffset = offset;
accumulatedDistance = offset;
}This means that when Suggested fix: add after if (shouldTrackDashOffset) {
accumulatedDistance = 0;
ctx.lineDashOffset = 0;
} |
||||||
|
|
||||||
| switch (lineType) { | ||||||
| case LineType.Simple: { | ||||||
| ctx.lineTo(currentX, currentY); | ||||||
| if (shouldTrackDashOffset) { | ||||||
| const prevItem = items[i - 1]; | ||||||
| const prevX = prevItem.x * horizontalPixelRatio; | ||||||
| const prevY = prevItem.y * verticalPixelRatio; | ||||||
| accumulatedDistance += distanceByCoordinates(prevX, prevY, currentX, currentY); | ||||||
| } | ||||||
| break; | ||||||
| } | ||||||
| case LineType.WithSteps: { | ||||||
| const prevItem = items[i - 1]; | ||||||
| const prevY = prevItem.y * verticalPixelRatio; | ||||||
| ctx.lineTo(currentX, prevY); | ||||||
| if (shouldTrackDashOffset) { | ||||||
|
|
@@ -120,7 +131,6 @@ export function walkLine<TItem extends LinePoint, TStyle extends CanvasRendering | |||||
| ); | ||||||
|
|
||||||
| if (shouldTrackDashOffset) { | ||||||
| const prevItem = items[i - 1]; | ||||||
| const prevX = prevItem.x * horizontalPixelRatio; | ||||||
| const prevY = prevItem.y * verticalPixelRatio; | ||||||
| const chord = distanceByCoordinates(prevX, prevY, currentX, currentY); | ||||||
|
|
||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test covers the
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| function generateData() { | ||
| const res = []; | ||
| const time = new Date(Date.UTC(2018, 0, 1, 0, 0, 0, 0)); | ||
| for (let i = 0; i < 30; ++i) { | ||
| const currentTime = time.getTime() / 1000; | ||
| time.setUTCDate(time.getUTCDate() + 1); | ||
|
|
||
| // Create a gap by pushing WhitespaceData (an empty point with just a time). | ||
| // This ensures the time scale has indices for these days, creating a gap in the series data's TimePointIndex array. | ||
| if (i > 10 && i < 20) { | ||
| res.push({ time: currentTime }); | ||
| continue; | ||
| } | ||
|
|
||
| res.push({ | ||
| time: currentTime, | ||
| value: 10 + Math.sin(i / 5) * 5, | ||
| }); | ||
| } | ||
| return res; | ||
| } | ||
|
|
||
| function runTestCase(container) { | ||
| const chart = window.chart = LightweightCharts.createChart(container, { | ||
| layout: { | ||
| attributionLogo: false, | ||
| }, | ||
| timeScale: { | ||
| barSpacing: 20, | ||
| }, | ||
| }); | ||
|
|
||
| const data = generateData(); | ||
|
|
||
| // Line Series with connectGaps: false | ||
| const lineSeries = chart.addSeries(LightweightCharts.LineSeries, { | ||
| color: '#2196F3', | ||
| lineWidth: 2, | ||
| connectGaps: false, | ||
| title: 'Line (No Gaps)', | ||
| }); | ||
| lineSeries.setData(data); | ||
|
|
||
| // Area Series with connectGaps: false (offset for visibility) | ||
| const areaSeries = chart.addSeries(LightweightCharts.AreaSeries, { | ||
| topColor: 'rgba(33, 150, 243, 0.4)', | ||
| bottomColor: 'rgba(33, 150, 243, 0.1)', | ||
| lineColor: '#2196F3', | ||
| connectGaps: false, | ||
| title: 'Area (No Gaps)', | ||
| }); | ||
| areaSeries.setData(data.map(d => d.value !== undefined ? { ...d, value: d.value + 10 } : d)); | ||
|
|
||
| // Baseline Series with connectGaps: false | ||
| const baselineSeries = chart.addSeries(LightweightCharts.BaselineSeries, { | ||
| baseValue: { type: 'price', price: 30 }, | ||
| topFillColor1: 'rgba(38, 166, 154, 0.28)', | ||
| topFillColor2: 'rgba(38, 166, 154, 0.05)', | ||
| topLineColor: 'rgba(38, 166, 154, 1)', | ||
| bottomFillColor1: 'rgba(239, 83, 80, 0.05)', | ||
| bottomFillColor2: 'rgba(239, 83, 80, 0.28)', | ||
| bottomLineColor: 'rgba(239, 83, 80, 1)', | ||
| connectGaps: false, | ||
| title: 'Baseline (No Gaps)', | ||
| }); | ||
| baselineSeries.setData(data.map(d => d.value !== undefined ? { ...d, value: d.value + 20 } : d)); | ||
|
|
||
| chart.timeScale().fitContent(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation should explain more precisely what constitutes a "gap". Users may be confused about the interaction with multiple series on the same chart. I'd suggest something like:
Same JSDoc update needed for the identical blocks in
AreaStyleOptionsandBaselineStyleOptions.