Skip to content

Refactoring: Adapt getTicks to increase reusability of code #3393

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.

Sign up for GitHub

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

Merged
nikolasrieble merged 13 commits into from
Feb 23, 2023
Merged

Refactoring: Adapt getTicks to increase reusability of code #3393

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
f9300ae
CartesianAxis: Add startEquidistant option
Feb 21, 2023
0d036db
Merge remote-tracking branch 'origin' into nikolas.axis.equidistant
Feb 22, 2023
8be78fe
Reuse existing code better
Feb 22, 2023
870db7e
Review: Improve comment
Feb 22, 2023
28384be
Review: Naming
Feb 22, 2023
c8cd40e
Handle negative interval gracefully
Feb 22, 2023
5687ab1
Apply suggestions from code review
nikolasrieble Feb 22, 2023
f237c59
Separate utils from getTicks
Feb 22, 2023
73ef784
Extensive testing
Feb 22, 2023
6673d05
Fix tests
Feb 22, 2023
788c725
Remove new feature from prefactoring PR
Feb 22, 2023
9bca9b1
Revert deletion of tests
Feb 22, 2023
bb57619
Review: Update comment
Feb 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/cartesian/CartesianAxis.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,14 @@ import { Layer } from '../container/Layer';
import { Text } from '../component/Text';
import { Label } from '../component/Label';
import { isNumber } from '../util/DataUtils';
import { CartesianViewBox, adaptEventsOfChild, PresentationAttributesAdaptChildEvent } from '../util/types';
import {
CartesianViewBox,
adaptEventsOfChild,
PresentationAttributesAdaptChildEvent,
CartesianTickItem,
} from '../util/types';
import { filterProps } from '../util/ReactUtils';
import { CartesianTickItem, getTicks } from './TickUtils';
import { getTicks } from './ticks/getTicks';

export interface CartesianAxisProps {
className?: string;
Expand Down
49 changes: 16 additions & 33 deletions src/cartesian/TickUtils.ts → src/cartesian/ticks/getTicks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,10 @@
import _ from 'lodash';
import { mathSign, isNumber } from '../util/DataUtils';
import { getStringSize } from '../util/DOMUtils';
import { Props as CartesianAxisProps } from './CartesianAxis';
import { TickItem } from '../util/types';
import { Global } from '../util/Global';

export interface CartesianTickItem extends TickItem {
tickCoord?: number;
tickSize?: number;
isShow?: boolean;
}

function getNumberIntervalTicks(ticks: CartesianTickItem[], interval: number) {
return ticks.filter((entry, i) => i % (interval + 1) === 0);
}
import { CartesianTickItem } from '../../util/types';
import { mathSign, isNumber } from '../../util/DataUtils';
import { getStringSize } from '../../util/DOMUtils';
import { Props as CartesianAxisProps } from '../CartesianAxis';
import { Global } from '../../util/Global';
import { getNumberIntervalTicks } from './utils';

function getTicksEnd({
ticks,
Expand Down Expand Up @@ -68,7 +59,7 @@ function getTicksEnd({
}
}

return result.filter(entry => entry.isShow);
return result;
}

function getTicksStart(
Expand Down Expand Up @@ -149,7 +140,7 @@ function getTicksStart(
}
}

return result.filter(entry => entry.isShow);
return result;
}

export function getTicks(props: CartesianAxisProps, fontSize?: string, letterSpacing?: string): any[] {
Expand All @@ -163,8 +154,10 @@ export function getTicks(props: CartesianAxisProps, fontSize?: string, letterSpa
return getNumberIntervalTicks(ticks, typeof interval === 'number' && isNumber(interval) ? interval : 0);
}

if (interval === 'preserveStartEnd') {
return getTicksStart(
let candidates: CartesianTickItem[] = [];

if (interval === 'preserveStart' || interval === 'preserveStartEnd') {

@petii petii Feb 22, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

My personal preference is for the early returns as in before the changes, but since you moved the isShow filtering here it makes sense to try and reduce code duplication.

However it could also make sense that to not stop here but create and use an additional layer like

function getShownTicks(...) {
  return getTicks(...).filter(entry => entry.isShow);
}

But then you have to go and change every usage of getTicks to keep existing behaviour (and it probably does not work too well with the following PR?).

Alternatively, if we would want to go one step further in reducing code duplication we could introduce a holder for these like

const tickGeneratorForInterval: Record<
  CartesianAxisProps['interval'],
  (args: Omit<CartesianAxisProps, 'tickMargin'>) => CartesianTickItem[]
> = { 
  'preserveEnd': getTicksEnd, 
  'preserveStart': getTicksStart, 
  'preserveStartEnd': (args) => getTicksStart(args, true),
}

and then just

  const candidates = tickGeneratorForInterval[interval]({ ... });
  return candidates.filter(entry => entry.isShow);

to get rid of hide the branching altogether

@nikolasrieble nikolasrieble Feb 23, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I like the ideas, and will most probably come back to them - but only when following up with this file after landing the feature enhancement. For now I want to only refactor as much as I need to, in order to nicely implement the equidistant ticks.

@petii petii Feb 23, 2023 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looking at the following PR this also has the benefit that adding the new option is just

  'equidistantPreserveStart': (args) => getEveryNthTick(getTicksStart(args)),

instead of essentially adding back the duplicated code that this PR removed. With that context I would argue even more so that the conditions should still be separate and not share the branch for 'preserveStart' and 'preserveStartEnd'.

candidates = getTicksStart(
{
ticks,
tickFormatter,
Expand All @@ -175,11 +168,10 @@ export function getTicks(props: CartesianAxisProps, fontSize?: string, letterSpa
fontSize,
letterSpacing,
},
true,
interval === 'preserveStartEnd',
);
}
if (interval === 'preserveStart') {
return getTicksStart({
} else {
candidates = getTicksEnd({
ticks,
tickFormatter,
viewBox,
Expand All @@ -191,14 +183,5 @@ export function getTicks(props: CartesianAxisProps, fontSize?: string, letterSpa
});
}

return getTicksEnd({
ticks,
tickFormatter,
viewBox,
orientation,
minTickGap,
unit,
fontSize,
letterSpacing,
});
return candidates.filter(entry => entry.isShow);
}
26 changes: 26 additions & 0 deletions src/cartesian/ticks/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
import { CartesianTickItem } from '../../util/types';

/**
* Given an array and a number N, return a new array which contains every nTh
* element of the input array. For n below 1, an empty array is returned.
* @param {T[]} array An input array.
* @param {integer} n A number
* @returns {T[]} The result array of the same type as the input array.
*/
function getEveryNth<Type>(array: Type[], n: number): Type[] {

@petii petii Feb 22, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This could be on it's own. Maybe in something like src/util/ArrayUtils.ts?

@ckifer ckifer Feb 23, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah this isn't 'tick' specific per se.

Also if we're going to move to different naming conventions for utils and for our files in general lets discuss.

We'll end up having files in util/SomeUtilFile.ts as well as places like cartesian/ticks/utils. Which pattern do we follow? imo we should stick to something consistent for ease of contribution

@nikolasrieble nikolasrieble Feb 23, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I find that things should be as close to their usage as possible. If used all across the places, a top level utils folder makes sense. But if used only within CartesianAxis, I would argue that is where it should live.

@petii petii Feb 23, 2023

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Right now it's only used in CartesianAxis. But when the need for the functionality comes along outside of tick or cartesian it's much easier to miss it if it's hidden a few subfolders deep. (Which then leads to just duplicating some similar logic ad-hoc where and when it's needed)

@nikolasrieble nikolasrieble Feb 23, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fair enough. For consistency I will move it back to a shared utils.

if (n < 1) {
return [];
}
if (n === 1) {
return array;
}
const result = [];
for (let i = 0; i < array.length; i += n) {
result.push(array[i]);
}
return result;
}

export function getNumberIntervalTicks(ticks: CartesianTickItem[], interval: number) {
return getEveryNth(ticks, interval + 1);
}
2 changes: 1 addition & 1 deletion src/chart/generateCategoricalChart.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import React, { Component, cloneElement, isValidElement, createElement } from 'react';
import classNames from 'classnames';
import _, { isArray, isBoolean, isNil } from 'lodash';
import { getTicks } from '../cartesian/TickUtils';
import { getTicks } from '../cartesian/ticks/getTicks';
import { Surface } from '../container/Surface';
import { Layer } from '../container/Layer';
import { Tooltip } from '../component/Tooltip';
Expand Down
6 changes: 6 additions & 0 deletions src/util/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1116,6 +1116,12 @@ export interface TickItem {
index?: number;
}

export interface CartesianTickItem extends TickItem {
tickCoord?: number;
tickSize?: number;
isShow?: boolean;
}

export interface Margin {
top?: number;
right?: number;
Expand Down
91 changes: 44 additions & 47 deletions test/cartesian/TickUtils.spec.ts → test/cartesian/ticks/getTicks.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { getTicks } from '../../src/cartesian/TickUtils';
import { getTicks } from '../../../src/cartesian/ticks/getTicks';

const EXAMPLE_INPUT = {
axisLine: true,
Expand All @@ -14,11 +14,12 @@ const EXAMPLE_INPUT = {
tickMargin: 2,
tickSize: 6,
ticks: [
{ value: 10, coordinate: 50 },
{ value: 1000, coordinate: 100 },
{ value: 20, coordinate: 150 },
{ value: 40, coordinate: 200 },
{ value: 90, coordinate: 250 },
{ value: '10', coordinate: 50 },
{ value: '1000', coordinate: 100 },
{ value: '20', coordinate: 150 },
{ value: '40', coordinate: 200 },
{ value: '90', coordinate: 250 },
{ value: 'A', coordinate: 300 },
],
length: 5,
viewBox: { x: 0, y: 0, width: 500, height: 500 },
Expand All @@ -27,9 +28,9 @@ const EXAMPLE_INPUT = {
y: 100,
};

jest.mock('../../src/util/DOMUtils', () => ({
jest.mock('../../../src/util/DOMUtils', () => ({
// We mock string size measurement, because getStringSize else returns 0 in these tests.
getStringSize: jest.fn(() => ({ width: 20, height: 20 })),
getStringSize: jest.fn((text: string) => ({ width: text.length, height: 20 })),

@ckifer ckifer Feb 23, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think this adds any indication of actual text length. Better maybe than no variation, but certainly not accurate.

@nikolasrieble nikolasrieble Feb 23, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agree, but this enables me to test with larger labels on different positions (i.e. first tick, second tick, third tick, etc.)

}));

// These tests have been generated by merely documenting existing behaviour.
Expand All @@ -42,11 +43,12 @@ describe('getTicks', () => {
const result = getTicks(input);

expect(result).toEqual([
{ value: 10, coordinate: 50, tickCoord: 50, isShow: true },
{ value: 1000, coordinate: 100, tickCoord: 100, isShow: true },
{ value: 20, coordinate: 150, tickCoord: 150, isShow: true },
{ value: 40, coordinate: 200, tickCoord: 200, isShow: true },
{ value: 90, coordinate: 250, tickCoord: 250, isShow: true },
{ value: '10', coordinate: 50, tickCoord: 50, isShow: true },
{ value: '1000', coordinate: 100, tickCoord: 100, isShow: true },
{ value: '20', coordinate: 150, tickCoord: 150, isShow: true },
{ value: '40', coordinate: 200, tickCoord: 200, isShow: true },
{ value: '90', coordinate: 250, tickCoord: 250, isShow: true },
{ value: 'A', coordinate: 300, tickCoord: 300, isShow: true },
]);
});

Expand All @@ -56,16 +58,17 @@ describe('getTicks', () => {
const result = getTicks(input);

expect(result).toEqual([
{ value: 10, coordinate: 50, tickCoord: 50, isShow: true },
{ value: 1000, coordinate: 100, tickCoord: 100, isShow: true },
{ value: 20, coordinate: 150, tickCoord: 150, isShow: true },
{ value: 40, coordinate: 200, tickCoord: 200, isShow: true },
{ value: 90, coordinate: 250, tickCoord: 250, isShow: true },
{ value: '10', coordinate: 50, tickCoord: 50, isShow: true },
{ value: '1000', coordinate: 100, tickCoord: 100, isShow: true },
{ value: '20', coordinate: 150, tickCoord: 150, isShow: true },
{ value: '40', coordinate: 200, tickCoord: 200, isShow: true },
{ value: '90', coordinate: 250, tickCoord: 250, isShow: true },
{ value: 'A', coordinate: 300, tickCoord: 300, isShow: true },
]);
});
});

describe('If not all ticks can be shown, the interval is respected', () => {
describe('The interval is respected', () => {
const viewBoxWithSmallWidth = { x: 0, y: 0, width: 30, height: 500 };

test.each([
Expand All @@ -74,7 +77,7 @@ describe('getTicks', () => {
[
{
coordinate: 50,
value: 10,
value: '10',
},
],
],
Expand All @@ -83,11 +86,11 @@ describe('getTicks', () => {
[
{
coordinate: 50,
value: 10,
value: '10',
},
{
coordinate: 200,
value: 40,
value: '40',
},
],
],
Expand All @@ -96,15 +99,15 @@ describe('getTicks', () => {
[
{
coordinate: 50,
value: 10,
value: '10',
},
{
coordinate: 150,
value: 20,
value: '20',
},
{
coordinate: 250,
value: 90,
value: '90',
},
],
],
Expand All @@ -113,34 +116,38 @@ describe('getTicks', () => {
[
{
coordinate: 50,
value: 10,
value: '10',
},
{
coordinate: 100,
value: 1000,
value: '1000',
},
{
coordinate: 150,
value: 20,
value: '20',
},
{
coordinate: 200,
value: 40,
value: '40',
},
{
coordinate: 250,
value: 90,
value: '90',
},
{
coordinate: 300,
value: 'A',
},
],
],
[
'preserveStartEnd' as const,
[
{
coordinate: 250,
coordinate: 300,
isShow: true,
tickCoord: 20,
value: 90,
tickCoord: 29.5,
value: 'A',
},
],
],
Expand All @@ -149,25 +156,15 @@ describe('getTicks', () => {
'preserveEnd' as const,
[
{
coordinate: 250,
coordinate: 300,
isShow: true,
tickCoord: 20,
value: 90,
tickCoord: 29.5,
value: 'A',
},
],
],
[-1, []],
[
undefined,
[
{
coordinate: 250,
isShow: true,
tickCoord: 20,
value: 90,
},
],
],
[undefined, [{ coordinate: 300, isShow: true, tickCoord: 29.5, value: 'A' }]],
])(`interval %s works`, (interval, expectedResult) => {
const input = {
...EXAMPLE_INPUT,
Expand Down