Skip to content
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

CartesianAxis: Improve interval option 'equidistantPreserveStart' #3768

Merged
merged 19 commits into from
Sep 22, 2023
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
55 changes: 55 additions & 0 deletions src/cartesian/getEquidistantTicks.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
import { isVisible } from '../util/TickUtils';
import { CartesianTickItem } from '../util/types';
import { getEveryNthWithCondition } from '../util/getEveryNthWithCondition';
import { Sign } from './getTicks';

export function getEquidistantTicks(
sign: Sign,
boundaries: { start: number; end: number },
getTickSize: (tick: CartesianTickItem, index: number) => number,
ticks: CartesianTickItem[],
minTickGap: number,
): CartesianTickItem[] {
const result = (ticks || []).slice();

const { start: initialStart, end } = boundaries;
let index = 0;
// Premature optimisation idea 1: Estimate a lower bound, and start from there.
// For now, start from every tick
let stepsize = 1;
let start = initialStart;

while (stepsize <= result.length) {
// Given stepsize, evaluate whether every stepsize-th tick can be shown.
// If it can not, then increase the stepsize by 1, and try again.

const entry = ticks?.[index];

// Break condition - If we have evaluate all the ticks, then we are done.
if (entry === undefined) {
return getEveryNthWithCondition(ticks, stepsize);
}

// Check if the element collides with the next element
const size = getTickSize(entry, index);

const tickCoord = entry.coordinate;
// We will always show the first tick.
const isShow = index === 0 || isVisible(sign, tickCoord, size, start, end);

if (!isShow) {
// Start all over with a larger stepsize
index = 0;
start = initialStart;
stepsize += 1;
}

if (isShow) {
// If it can be shown, update the start
start = tickCoord + sign * (size / 2 + minTickGap);
index += stepsize;
}
}

return [];
}
7 changes: 6 additions & 1 deletion src/cartesian/getTicks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,9 @@ import {
getNumberIntervalTicks,
getAngledTickWidth,
} from '../util/TickUtils';
import { getEquidistantTicks } from './getEquidistantTicks';

type Sign = 0 | 1 | -1;
export type Sign = 0 | 1 | -1;

function getTicksEnd(
sign: Sign,
Expand Down Expand Up @@ -136,6 +137,10 @@ export function getTicks(props: CartesianAxisProps, fontSize?: string, letterSpa
const sign = ticks.length >= 2 ? mathSign(ticks[1].coordinate - ticks[0].coordinate) : 1;
const boundaries = getTickBoundaries(viewBox, sign, sizeKey);

if (interval === 'equidistant') {
return getEquidistantTicks(sign, boundaries, getTickSize, ticks, minTickGap);
}

if (interval === 'equidistantPreserveStart') {
candidates = getTicksStart(sign, boundaries, getTickSize, ticks, minTickGap);

Expand Down
9 changes: 8 additions & 1 deletion src/util/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1120,8 +1120,15 @@ export interface BaseAxisProps {
* 'preserveStartEnd' keeps the left tick on collision and ensures that the first and last ticks are always shown.
* 'equidistantPreserveStart' computes which ticks are shown according to the 'preserveStart' strategy and
* then selects a number N such that every nTh tick will be shown.
* 'equidistant' selects a number N such that every nTh tick will be shown without collision.
*/
export type AxisInterval = number | 'preserveStart' | 'preserveEnd' | 'preserveStartEnd' | 'equidistantPreserveStart';
export type AxisInterval =
| number
| 'preserveStart'
| 'preserveEnd'
| 'preserveStartEnd'
| 'equidistantPreserveStart'
| 'equidistant';

export interface TickItem {
value?: any;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,27 @@ export default {

export const TickPositioning = {
render: () => {
const intervalOptions = ['preserveStart', 'preserveEnd', 'preserveStartEnd', 'equidistantPreserveStart', 0];
const intervalOptions = [
'preserveStart',
'preserveEnd',
'preserveStartEnd',
'equidistantPreserveStart',
'equidistant',
0,
];

return (
<ResponsiveContainer>
<LineChart data={ticks}>
<LineChart
data={ticks}
// Margins are neccessary to show ticks that extend beyond the chart (i.e. last and first tick).
margin={{
top: 20,
right: 30,
left: 20,
bottom: 20,
}}
>
<Line dataKey="coordinate" />
{intervalOptions.map((intervalOption, index) => (
<XAxis
Expand Down
40 changes: 40 additions & 0 deletions test/cartesian/getEquidistantTicks.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
// unit test for getEquidisantTicks

import _ from 'lodash';
nikolasrieble marked this conversation as resolved.
Show resolved Hide resolved
import { getEquidistantTicks } from '../../src/cartesian/getEquidistantTicks';
import { CartesianTickItem } from '../../src/util/types';

describe('getEquidistantTicks', () => {
// We mock getting the tick width by simply using the value of the tick.
// This makes testing rather easy and intuitive.
const getTickSize = (tick: CartesianTickItem) => {
return tick.value;
};

it('should return empty array if no ticks are passed', () => {
const result = getEquidistantTicks(1, { start: 0, end: 100 }, getTickSize, [], 0);
expect(result).toEqual([]);
});

test.each([
// If all ticks fit, we show all ticks
{ ticksThatFit: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9], resultingTicks: [0, 1, 2, 3, 4, 5, 6, 7, 8, 9] },
{ ticksThatFit: [0, 1, 2, 3, 4, 5, 6, 7, 8], resultingTicks: [0, 2, 4, 6, 8] },
{ ticksThatFit: [0, 1, 2, 3, 4, 5, 6, 7], resultingTicks: [0, 5] },
{ ticksThatFit: [0, 1, 2, 3, 4, 5, 6], resultingTicks: [0, 5] },
{ ticksThatFit: [0, 1, 2, 3, 4, 5], resultingTicks: [0, 5] },
{ ticksThatFit: [0, 1, 2, 3, 4], resultingTicks: [0] },
Comment on lines +20 to +25
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section deserves scrutiny. Is this the behaviour we want to achieve?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this really true in the browser though?

I feel like all of these should be able to fit? every time?

I kind of think we shouldn't always preserve start and instead show as many as we can while maintaining equidistance

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I construct the ticks in the test such that this is true in the browser.
But good point, if we were to dynamically allow starting from any tick but still only show equidistant ticks, we could show much more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really, we could consider to redo the interface completely. As far as I understand it, the options currently are:

alwaysShowStart alwaysShowEnd collisionHandlingStrategy currentName
true false keep left preserveStart
false true keep right preserveEnd
true true keep left preserveStartEnd
true false compute collision to direct neighbour and then select every nTH equidistantPreserveStart
true false show every nTH without collision equidistant

We could have the option to choose equidistant without alwaysShowStart.

I admittedly find the naming of these strategies not that insightful. We should probably investigate how others do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The more I think about this, I believe we should simply replace the existing equidistantPreserveStart with this implementation.

This would be a breaking change, as it changes existing behaviour, but the change would only be to the better, namely we would show more ticks than we previously did, and there would yet be no collision.

@ckifer What do you think about this approach?

I like it, because it does not further complicate the API and it improves the existing behaviour (almost a bug fix)

Copy link
Member

@ckifer ckifer Sep 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm on board - if people complain that their chart looks better then... not sure lol. If anything we could do the same thing with preserveEnd if we wanted no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thanks for the discussion.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm on board - of people complain that their chart looks better then... not sure lol. If anything we could do the same thing with preserveEnd if we wanted no?

They will look better, Coltin! :P

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly 🚀. If there are complaints I will say but... they look better 😂

{ ticksThatFit: [0, 1, 2, 3], resultingTicks: [0] },
{ ticksThatFit: [0, 1, 2], resultingTicks: [0] },
{ ticksThatFit: [0, 1], resultingTicks: [0] },
{ ticksThatFit: [0], resultingTicks: [0] },
// We always show the first tick, even if it does not fit
{ ticksThatFit: [1, 2, 3, 4, 5, 6, 7, 8, 9], resultingTicks: [0] },
])('Show only every n-th tick that fits, but always show the first.', ({ ticksThatFit, resultingTicks }) => {
const ticks = _.range(0, 10).map(index => {
return { value: ticksThatFit.includes(index) ? 10 : 1000, coordinate: index * 50 };
});
const result = getEquidistantTicks(1, { start: 0, end: 10 * 50 }, getTickSize, ticks, 0);
expect(result).toEqual(resultingTicks.map(index => ticks[index]));
});
});