Conversation
78bb493 to
b9167f6
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate issues affect datetime intervals, inverted/rotated ranges, and scrollbar break translation.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Fixes chart scrollbar positioning and panning across argument-axis scale breaks.
Changes:
- Adds break-aware range and panning corrections.
- Updates scrollbar translation and drag handling.
- Extends mocks and regression/integration tests.
File summaries
| File | Summary |
|---|---|
packages/devextreme/testing/tests/DevExpress.viz.charts/zoomAndPan.tests.js |
Adds panning regression coverage; the new path is not exercised by the synthetic event pair. |
packages/devextreme/testing/tests/DevExpress.viz.charts/scrollBar.tests.js |
Updates scrollbar initialization tests. |
packages/devextreme/testing/tests/DevExpress.viz.charts/chart.part5.tests.js |
Updates scrollbar mock expectations. |
packages/devextreme/testing/tests/DevExpress.viz.charts/chart.integration.tests.js |
Adds scrollbar scale-break integration tests. |
packages/devextreme/testing/helpers/chartMocks.js |
Extends axis mock support. |
packages/devextreme/js/__internal/viz/chart_components/zoom_and_pan.ts |
Applies corrected ranges during panning. |
packages/devextreme/js/__internal/viz/chart_components/scroll_bar.ts |
Maps scrollbar positions to ranges; moderate issues remain for data ordering and break widths. |
packages/devextreme/js/__internal/viz/chart_components/base_chart.ts |
Passes whole-range breaks to the scrollbar. |
packages/devextreme/js/__internal/viz/axes/base_axis.ts |
Adds break-aware calculations; moderate issues remain for numeric datetime intervals and inverted ranges. |
Review details
Suppressed comments (3)
packages/devextreme/js/__internal/viz/axes/base_axis.ts:1307
- Datetime
TimeIntervalConfigaccepts numeric millisecond intervals, andforceUserTickIntervalpreserves such a number in_tickInterval.dateUtils.dateToMilliseconds(number)returns 0, so these new range-length calculations treat a user break as fully hidden instead of retaining one tick interval; the scrollbar thumb and panning scale are incorrect for datetime axes with a numerictickInterval. Return numeric intervals unchanged and only convert interval objects.
return this._options.dataType === 'datetime' ? dateUtils.dateToMilliseconds(tickInterval) : tickInterval;
packages/devextreme/js/__internal/viz/axes/base_axis.ts:1371
adjustPannedRangereceiveszoom.min/zoom.maxdirectly, butTranslator2D.zoom()returns them in screen order for an inverted continuous axis, sostartValuecan be greater thanendValue._getBreaksForRangedoes not normalize viewport bounds and_filterBreaksthen finds no breaks, causing this early return to skip the scale-preserving correction. Panning or dragging the scrollbar across a break on an inverted axis can therefore change the visible duration; normalize the range before break detection and correction.
|| !this._getBreaksForRange(range.startValue, range.endValue).length) {
packages/devextreme/js/__internal/viz/chart_components/scroll_bar.ts:160
wholeRangeBreakscarriescumulativeWidthvalues computed from the axisbreakStyle.width, but this translator is configured withbreaksSize: 0.Translator2Dstill usescumulativeWidthwhen calculating its ratio and converting positions, so user-defined breaks leave a gap in the scrollbar track and make the thumb length differ from the axis visible-range fraction. Clear or recomputecumulativeWidthfor the scrollbar-specific break list before passing it to the translator.
breaks: wholeRangeBreaks?.length ? wholeRangeBreaks : null,
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
11c5aee to
fd1e450
Compare
There was a problem hiding this comment.
🟡 Changes recommended
One or more issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
packages/devextreme/js/__internal/viz/axes/base_axis.ts:1377
- This correction is skipped as soon as the newly computed range no longer contains a break. If a pan starts with a break in the viewport and then moves past that break, the early return leaves the data span changed, so the chart zooms out/in instead of preserving the original visible scale. The condition should also consider breaks in
storedParams.startRange(or run the correction for every continuous range whose starting range had a break).
if (!storedParams || type === constants.discrete || type === constants.logarithmic
|| !this._getBreaksForRange(range.startValue, range.endValue).length) {
return range;
packages/devextreme/js/__internal/viz/chart_components/scroll_bar.ts:119
- This ordering check is not valid for discrete axes:
from()returns categories in axis order, but>compares the category values themselves. For example, a non-inverted axis with categories['B', 'A']produces{ startValue: 'A', endValue: 'B' }, andadjustRangedoes not normalize discrete ranges, so dragging the scrollbar can reverse the visible category range. Determine the order from the translator's inversion state (or the category indices), rather than comparing values.
return from > to ? { startValue: to, endValue: from } : { startValue: from, endValue: to };
packages/devextreme/js/__internal/viz/chart_components/zoom_and_pan.ts:641
- The end handler applies the explicit scrollbar range without checking whether argument-axis panning is enabled. Thus, even if the move branch is guarded, releasing a scrollbar drag still pans once for charts configured with
argumentAxis: 'zoom'ornone; retain the samezoomAndPan.options.argumentAxis.pancondition here.
if (e.scrollRange) {
panArgumentAxisToThumb(e, e.scrollRange);
}
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Unresolved issues remain with category ordering, inverted-axis handling, and pan-option enforcement.
Review details
Suppressed comments (3)
packages/devextreme/js/__internal/viz/chart_components/scroll_bar.ts:119
fromandtoare category values for a discrete translator, so comparing them with>does not describe their order on the axis. For categories such as['Z', 'A', 'M'], this reverses the range emitted by every real scrollbar drag and the newscrollRangepath can pan to the wrong categories. Use the translator's screen/data direction to choose the endpoints instead of comparing category values.
// on an inverted axis the bar runs against the data, so the ends are reported in data order
return from > to ? { startValue: to, endValue: from } : { startValue: from, endValue: to };
packages/devextreme/js/__internal/viz/chart_components/zoom_and_pan.ts:276
- The scrollbar range is normalized to data order above, but on an inverted axis the physical start of the thumb corresponds to
scrollRange.endValue, notstartValue. Passing the hard-coded'start'anchor makes the scale-break correction preserve the wrong edge, so dragging an inverted scrollbar across a break can shift or resize the range instead of following the thumb. Select'end'when the axis translator is inverted.
const getRange = (axis) => axis.adjustRange(
axis.adjustPannedRange(getVizRangeObject([scrollRange.startValue, scrollRange.endValue]), 'start'),
);
packages/devextreme/js/__internal/viz/chart_components/zoom_and_pan.ts:272
- This path now handles every real scrollbar drag, but it bypasses the
options.argumentAxis.pancheck used by the regular scrollbar path. WhenzoomAndPan.argumentAxisis"zoom"or"none", the start handler only marks the event as cancelled; the move handler still callsaxisZoomfor every argument axis and pans the chart. Guard this path with the pan option so dragging a scrollbar cannot enable panning that the option disables.
const axes = getFilteredAxes(chart._argumentAxes);
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
fd1e450 to
fd989bc
Compare
There was a problem hiding this comment.
🟡 Changes recommended
One or more issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
packages/devextreme/js/__internal/viz/axes/base_axis.ts:1325
- This manual half-tick correction does not match the axis normalization:
tick_generatorsnaps each user-break boundary to a nearby tick viagetBaseTickbefore applying the ±half-interval correction, while this code shifts the raw boundaries directly. For breaks not aligned to the tick grid, the scrollbar hides a different interval from the chart axis, so thumb geometry and panning scale diverge. Reuse the normalized break values or centralize that normalization.
return this._getBreaksForRange(businessRange.min, businessRange.max)
.reduce((result, scaleBreak) => {
const hidden = this._getHiddenDuration(scaleBreak, interval);
// a gap is hidden whole, so its shift is zero and it is taken as is
const shift = ((scaleBreak.to - scaleBreak.from) - hidden) / 2;
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
fd989bc to
5593d9d
Compare
5593d9d to
d436f6a
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Three moderate issues remain in scrollbar endpoint mapping and drag handling or coverage.
Review details
Suppressed comments (3)
packages/devextreme/js/__internal/viz/chart_components/scroll_bar.ts:116
- For a discrete argument axis,
initpassesstick: falsewhenvalueMarginsEnabledis at its default, sosetPositiondeliberately places the thumb's end at the boundary after the last visible category. Callingtranslator.from(start + length)with the default direction0converts that boundary to the next category (for example, a visible range[3, 7]becomes[3, 8]), expanding the range on every real scrollbar drag. Map each endpoint with the corresponding direction/offset used bysetPosition(including the inverted case), and add a discrete scrollbar-drag regression test.
const from = translator.from(start);
const to = translator.from(start + length);
packages/devextreme/js/__internal/viz/chart_components/zoom_and_pan.ts:634
- The new
scrollRangebranch is only covered by scrollbar tests usingworkdaysOnly. The user-defined-break cases exercise programmatic range changes and the ordinary chart pointer, but never drag the real scrollbar acrossargumentAxis.breaks; a regression in translating a user-defined whole-range break toscrollRangewould therefore pass. Please add a direct scrollbar-drag case with a user-defined break and assert the range/scale after crossing it.
if (e.scrollRange && options.argumentAxis.pan) {
panArgumentAxisToThumb(e, e.scrollRange);
packages/devextreme/js/__internal/viz/chart_components/zoom_and_pan.ts:643
- This remaps the axis from the scrollbar position a second time on every drag end, including a start/end gesture with no move. When the visual range boundary is inside a zero-width scale break,
_getRangeAtPositionresolves that boundary to the value after the break, so simply clicking the thumb can change the chart range even though the thumb did not move. Apply the thumb range only for an actual move (or preserve the current range for a zero-offset end event).
if (e.scrollRange && options.argumentAxis.pan) {
panArgumentAxisToThumb(e, e.scrollRange);
}
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
d436f6a to
14ce35c
Compare
14ce35c to
7a137f6
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Whole-range break calculations use raw option breaks instead of tick-normalized breaks, causing incorrect scrollbar sizing or panning for some numeric and non-aligned breaks.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
Logarithmic argument-axis breaks are explicitly excluded from the new correction and scrollbar mapping.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
packages/devextreme/js/__internal/viz/axes/base_axis.ts:1360
- The corresponding early return here also makes
adjustPannedRangetreat a logarithmic range as if it had no breaks. Even if the scrollbar is taught about the whole-range breaks above, panning across a supported logarithmic break will not preserve the visible scale, so this path needs the same log-space break-length handling.
packages/devextreme/js/__internal/viz/axes/base_axis.ts:1320
- This guard skips all breaks for logarithmic axes, even though the axis translator and tick generator support logarithmic scale breaks and
argumentAxis.breaksis a public option. A logarithmic argument axis with breaks therefore still builds a break-free scrollbar translator and can retain the jump this change is intended to fix. Please handle break lengths in the logarithmic coordinate space instead of excluding this axis type.
if (type === constants.discrete || type === constants.logarithmic
|| !isDefined(businessRange.min) || !isDefined(businessRange.max)) {
return [];
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
No description provided.