Skip to content

Commit

Permalink
fix(axis): rotated horizontal xAxisHeight is calculated correctly aft…
Browse files Browse the repository at this point in the history
…er loading new data

'maxTickWidths.x.ticks' have to be cached before calculating xAxisHeight
for correct xAxisHeight calculation.
By updating XAxisTickClip BEFORE calculating xAxisHeight we can make use of a side
effect, which will cache 'maxTickWidths.x.ticks'.

The side effect is caused by following call stack:
- updateXAxisTickClip() --> getHorizontalAxisHeight() --> $$.axis.getMaxTickWidth()
- In this case, 'maxTickWidths.x.ticks' will be calculated and cached, and XAxisTickClip
will be updated.

Empty 'maxTickWidths.x.ticks' before recalculation to prevent keeping old state
after new data is loaded into the chart.

Added 'axis_x_tick_count' for tick count calculation in 'getAxisTickRotate()' as
setting tick count manually would break the autorotation.

Added extra checks for places in the code where values could have been divided by
zero to avoid runtime errors.

Added extra condition for tick offset calculation.

There was a missing check if every 'currentTicksMax.domain' is greater than zero.
Due to this, if new data was laoded with 'unload: true' it could happen, that
'currentTicksMax.domain' would be [0,0].
In this case, even if the domain is not same, it would always result in returning
'currentTicksMax.size'and not updating 'currentTicksMax.domain' which resulted
in not updating the rotation on x axis.

Calculating tick offset is only necessary when setting 'axis_x_tick_count'
on x axis type 'category'.
It should be calculated only if lower or equal than the actual amount of
filtered targets that should have been shown.

Fix #1786
Close #1787
  • Loading branch information
michkami authored and netil committed Dec 2, 2020
1 parent 4660d3a commit ef2754f
Show file tree
Hide file tree
Showing 7 changed files with 75 additions and 41 deletions.
27 changes: 17 additions & 10 deletions demo/demo.js
Original file line number Diff line number Diff line change
Expand Up @@ -1239,17 +1239,20 @@ var demos = {
autorotate: true,
rotate: 15,
culling: false
}
},
clipPath: false
}
}
},
func: function(chart) {
chart.timer = [
setTimeout(function() {
chart.resize({width: window.innerWidth * 0.4});
}, 1000),
setTimeout(function() {
chart.resize({width: window.innerWidth * 0.4});
}, 1000),

setTimeout(function() {chart.resize();}, 3000)
setTimeout(function() {
chart.resize();
}, 3000)
];
}
},
Expand Down Expand Up @@ -1294,6 +1297,7 @@ setTimeout(function() {chart.resize();}, 3000)
},
axis: {
x: {
type: "timeseries",
tick: {
fit: true,
multiline: false,
Expand All @@ -1303,16 +1307,19 @@ setTimeout(function() {chart.resize();}, 3000)
count: 8,
format: "%m-%d-%Y %H:%M:%S"
},
type: "timeseries"
clipPath: false
}
}
},
func: function(chart) {
chart.timer = [
setTimeout(function() {
chart.resize({width: window.innerWidth * 0.4});
}, 1000),
setTimeout(function() {chart.resize();}, 3000)
setTimeout(function() {
chart.resize({width: window.innerWidth * 0.4});
}, 1000),

setTimeout(function() {
chart.resize();
}, 3000)
];
}
}
Expand Down
35 changes: 26 additions & 9 deletions src/ChartInternal/Axis/Axis.ts
Original file line number Diff line number Diff line change
Expand Up @@ -576,15 +576,23 @@ class Axis {
const scale = $$.scale[id].copy().domain($$[`get${isYAxis ? "Y" : "X"}Domain`](targetsToShow, id));
const domain = scale.domain();

// do not compute if domain is same
if ((domain[0] === domain[1] && domain.every(v => v > 0)) ||
(isArray(currentTickMax.domain) && currentTickMax.domain[0] === currentTickMax.domain[1])
) {
const isDomainSame = domain[0] === domain[1] && domain.every(v => v > 0);
const isCurrentMaxTickDomainSame = isArray(currentTickMax.domain) &&
currentTickMax.domain[0] === currentTickMax.domain[1] &&
currentTickMax.domain.every(v => v > 0);

// do not compute if domain or currentMaxTickDomain is same
if (isDomainSame || isCurrentMaxTickDomainSame) {
return currentTickMax.size;
} else {
currentTickMax.domain = domain;
}

// reset old max state value to prevent from new data loading
if (!isYAxis) {
currentTickMax.ticks.splice(0);
}

const axis = this.getAxis(id, scale, false, false, true);
const tickCount = config[`axis_${id}_tick_count`];
const tickValues = config[`axis_${id}_tick_values`];
Expand Down Expand Up @@ -618,7 +626,7 @@ class Axis {

maxWidth = Math.max(maxWidth, currentTextWidth);
// cache tick text width for getXAxisTickTextY2Overflow()
if (id === "x") {
if (!isYAxis) {
currentTickMax.ticks[i] = currentTextWidth;
}
});
Expand Down Expand Up @@ -691,9 +699,13 @@ class Axis {
maxOverflow = Math.max(maxOverflow, overflow);
}

const filteredTargets = $$.filterTargetsToShow($$.data.targets);
let tickOffset = 0;

if (!isTimeSeries) {
if (
!isTimeSeries &&
config.axis_x_tick_count <= filteredTargets.length && filteredTargets[0].values.length
) {
const scale = getScale($$.axis.getAxisType("x"), 0, widthWithoutCurrentPaddingLeft - maxOverflow)
.domain([
left * -1,
Expand Down Expand Up @@ -729,10 +741,15 @@ class Axis {
const timeDiff = lastX - firstX;

const range = timeDiff + padding.left + padding.right;
const relativeTickWidth = (timeDiff / tickCount) / range;
let left = 0;
let right = 0;

if (tickCount && range) {
const relativeTickWidth = (timeDiff / tickCount) / range;

const left = padding.left / range / relativeTickWidth || 0;
const right = padding.right / range / relativeTickWidth || 0;
left = padding.left / range / relativeTickWidth;
right = padding.right / range / relativeTickWidth;
}

padding = {left, right};
}
Expand Down
17 changes: 11 additions & 6 deletions src/ChartInternal/internals/size.axis.ts
Original file line number Diff line number Diff line change
Expand Up @@ -96,13 +96,18 @@ export default {
let rotate = config[`axis_${id}_tick_rotate`];

if (id === "x") {
const isCategorized = axis.isCategorized();
const isTimeSeries = axis.isTimeSeries();
const allowedXAxisTypes = isCategorized || isTimeSeries;
let tickCount = 0;
const allowedXAxisTypes = axis.isCategorized() || axis.isTimeSeries();

if (config.axis_x_tick_fit && allowedXAxisTypes) {
tickCount = state.current.maxTickWidths.x.ticks.length + (isTimeSeries ? -1 : 1);
const xTickCount = config.axis_x_tick_count;
const currentXTicksLength = state.current.maxTickWidths.x.ticks.length;
let tickCount = 0;

if (xTickCount) {
tickCount = xTickCount > currentXTicksLength ? currentXTicksLength : xTickCount;
} else if (currentXTicksLength) {
tickCount = currentXTicksLength;
}

if (tickCount !== state.axis.x.tickCount) {
state.axis.x.padding = $$.axis.getXAxisPadding(tickCount);
Expand Down Expand Up @@ -140,7 +145,7 @@ export default {
axis.x.padding.left + axis.x.padding.right;

const maxTickWidth = $$.axis.getMaxTickWidth("x");
const tickLength = (xAxisLength / tickCountWithPadding) || 0;
const tickLength = tickCountWithPadding ? xAxisLength / tickCountWithPadding : 0;

return maxTickWidth > tickLength;
}
Expand Down
8 changes: 4 additions & 4 deletions src/ChartInternal/internals/size.ts
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,10 @@ export default {
height: legend ? $$.getLegendHeight() : 0
};

if (!hasArc && config.axis_x_show && config.axis_x_tick_autorotate) {
$$.updateXAxisTickClip();
}

const legendHeightForBottom = state.isLegendRight || state.isLegendInset ? 0 : currLegend.height;
const xAxisHeight = isRotated || hasArc ? 0 : $$.getHorizontalAxisHeight("x");

Expand Down Expand Up @@ -336,9 +340,5 @@ export default {
if (state.isLegendRight && hasArc) {
state.margin3.left = state.arcWidth / 2 + state.radiusExpanded * 1.1;
}

if (!hasArc && config.axis_x_show && config.axis_x_tick_autorotate) {
$$.updateXAxisTickClip();
}
}
};
4 changes: 3 additions & 1 deletion src/config/Options/axis/x.ts
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,7 @@ export default {
* - axis.x.tick.multiline=false
* - axis.x.tick.culling=false
* - axis.x.tick.fit=true
* - **NOTE:** axis.x.tick.clippath=false is necessary for calculating the overflow padding between the end of x axis and the width of the SVG
* @name axis․x․tick․autorotate
* @memberof Options
* @type {boolean}
Expand All @@ -326,7 +327,8 @@ export default {
* multiline: false,
* culling: false,
* fit: true
* }
* },
* clipPath: false
* }
* }
*/
Expand Down
24 changes: 13 additions & 11 deletions test/internals/axis-spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1128,7 +1128,8 @@ describe("AXIS", function() {
centered: false,
culling: false,
multiline: false
}
},
clipPath: false
}
}
};
Expand Down Expand Up @@ -1172,13 +1173,13 @@ describe("AXIS", function() {
expect(tspan.attr("dx")).to.be.equal("2.070552360820166");
});

compare(15, 45.525421142578125, 56.82012354874871, 109.4987923936019)
compare(15, 45, 56, 71)
});

it("should resize when all data hidden", () => {
it("should not resize x axis when all data hidden", () => {
chart.hide("data1");

compare(args.axis.x.tick.rotate, 6, 57, 110);
compare(args.axis.x.tick.rotate, 6, 57, 71);

chart.show("data1");
});
Expand Down Expand Up @@ -1218,31 +1219,31 @@ describe("AXIS", function() {
});

it("should be above 0 if rotated", () => {
compareOverflow(109.4987923936019);
compareOverflow(71);
});

it("update config", () => {
args.axis.x.padding = {right: 2};
});

it("should be defaultPadding + tickOffset if padding right is set", () => {
compareOverflow(defaultPadding + 33);
it("should be defaultPadding if padding right is set", () => {
compareOverflow(defaultPadding);
});

it("update config", () => {
args.axis.x.padding = {left: 2};
});

it("should be above defaultPadding if padding left is set", () => {
compareOverflow( 109.38116563068422);
compareOverflow( 80);
});

it("update config", () => {
args.axis.x.padding = {left: 2, right: 2};
});

it("should be above defaultPadding if padding is set", () => {
compareOverflow(37);
it("should be equal to defaultPadding if padding is set", () => {
compareOverflow(defaultPadding);
});
});

Expand Down Expand Up @@ -1282,6 +1283,7 @@ describe("AXIS", function() {
count: 5,
format: "%Y-%m-%d %H:%M:%S",
},
clipPath: false
}
}
};
Expand Down Expand Up @@ -1319,7 +1321,7 @@ describe("AXIS", function() {
compare(15, 45.145263671875, 56.439983076254386, 108.67536019184263)
});

it("should resize when all data hidden", () => {
it("should not resize x axis when all data hidden", () => {
chart.hide("Temperature");

compare(args.axis.x.tick.rotate, 6, 57, 108);
Expand Down
1 change: 1 addition & 0 deletions types/axis.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,7 @@ export interface XTickConfiguration {
* - axis.x.tick.multiline=false
* - axis.x.tick.culling=false
* - axis.x.tick.fit=true
* - **NOTE:** axis.x.tick.clippath=false is necessary for calculating the overflow padding between the end of x axis and the width of the SVG
*/
autorotate?: boolean;

Expand Down

0 comments on commit ef2754f

Please sign in to comment.