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

feat(axis): add tickLabelPadding prop #217

Merged
merged 22 commits into from
Jul 3, 2019
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
f3032fe
feat(specs.ts stories/axis.tsx): included tickLabelPadding prop
rshen91 Mar 20, 2019
be36cb1
feat(axis.tsx axis_utils.ts): tickLabelPadding extend width of Text Axis
rshen91 Mar 22, 2019
451ecfb
feat(axis): add tickLabelPadding prop
rshen91 Apr 17, 2019
c092953
feat(styling and axis): supress canvas padding manipulation
rshen91 Apr 23, 2019
ab3869a
feat(axis_utils canvas_bbox): add theme tickLabelPadding
rshen91 Apr 25, 2019
9bfa9b1
style(rendering.ts and test): define padding as default to 1
rshen91 May 20, 2019
0a3f443
style(axis.tsx): improve comment line 32
rshen91 May 21, 2019
b5dee48
refactor(stories/axis): add tickLabelPadding to tick label rotation axes
rshen91 May 21, 2019
2dfb614
feat(axis styling.tsx): add tickLabelPadding story theme and prop
rshen91 May 22, 2019
929b0ac
style(axis.ts rendering.ts): min to -90 readability and comments
rshen91 May 23, 2019
0f39133
refactor(styling): isolate spec and theme in tickLabelPadding story
rshen91 May 23, 2019
edb1f4b
refactor(canvas_bbox dark_theme): add validation for padding
rshen91 Jun 4, 2019
83ee9b7
Merge remote-tracking branch 'upstream/master' into rshen-94
rshen91 Jun 4, 2019
2dab12a
Merge branch 'master' into pr/217
markov00 Jun 10, 2019
7bba2fa
refactor: merge commit
rshen91 Jun 24, 2019
f8e8f5f
refactor: missed adding merge commits
rshen91 Jun 24, 2019
1507d9c
test(axis_utils.test.ts): add test to confirm positive padding
rshen91 Jun 25, 2019
fac9f17
Merge remote-tracking branch 'upstream/master' into rshen-94
rshen91 Jul 1, 2019
776239a
refactor(axis_utils specs): create style object
rshen91 Jul 2, 2019
d8dd8f4
test(axis_utils.test): add test for tickLabelPadding style prop or theme
rshen91 Jul 3, 2019
83fcc7d
refactor(axis_utils): refactor getAxisTickLabelPadding
rshen91 Jul 3, 2019
725177f
refactor(axis_utils): improve if statement
rshen91 Jul 3, 2019
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
12 changes: 11 additions & 1 deletion src/components/react_canvas/axis.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,16 @@ export class Axis extends React.PureComponent<AxisProps> {
return this.renderAxis();
}
renderTickLabel = (tick: AxisTick, i: number) => {
const { padding, ...labelStyle } = this.props.chartTheme.axes.tickLabelStyle;
/**
* padding is already computed through width
* and bbox_calculator using tickLabelPadding
* set padding to 0 to avoid conflict
*/
const labelStyle = {
...this.props.chartTheme.axes.tickLabelStyle,
padding: 0,
};

const {
axisSpec: { tickSize, tickPadding, position },
axisTicksDimensions,
Expand All @@ -48,6 +57,7 @@ export class Axis extends React.PureComponent<AxisProps> {
);

const { maxLabelTextWidth, maxLabelTextHeight } = axisTicksDimensions;

const centeredRectProps = centerRotationOrigin(axisTicksDimensions, {
x: tickLabelProps.x,
y: tickLabelProps.y,
Expand Down
13 changes: 12 additions & 1 deletion src/lib/axes/axis_utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -458,7 +458,7 @@ describe('Axis computational utils', () => {
});
test('should get max bbox dimensions for a tick in comparison to previous values', () => {
const bboxCalculator = new CanvasTextBBoxCalculator();
const reducer = getMaxBboxDimensions(bboxCalculator, 16, 'Arial', 0);
const reducer = getMaxBboxDimensions(bboxCalculator, 16, 'Arial', 0, 1);

const accWithGreaterValues = {
maxLabelBboxWidth: 100,
Expand Down Expand Up @@ -1272,4 +1272,15 @@ describe('Axis computational utils', () => {
expect(isBounded(lowerBounded)).toBe(true);
expect(isBounded(upperBounded)).toBe(true);
});
test('should not allow negative padding', () => {
rshen91 marked this conversation as resolved.
Show resolved Hide resolved
const negativePadding = -2;
// value canvas_text_bbox_calculator changes negative values is 1
const positivePadding = 1;

const bboxCalculator = new CanvasTextBBoxCalculator();
const negativeReducer = getMaxBboxDimensions(bboxCalculator, 16, 'Arial', 0, negativePadding);
const positiveReducer = getMaxBboxDimensions(bboxCalculator, 16, 'Arial', 0, positivePadding);

expect(JSON.stringify(negativeReducer)).toEqual(JSON.stringify(positiveReducer));
});
});
16 changes: 12 additions & 4 deletions src/lib/axes/axis_utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ export interface TickLabelProps {
/**
* Compute the ticks values and identify max width and height of the labels
* so we can compute the max space occupied by the axis component.
* @param axisSpec tbe spec of the axis
* @param axisSpec the spec of the axis
* @param xDomain the x domain associated
* @param yDomain the y domain array
* @param totalBarsInCluster the total number of grouped series
Expand All @@ -69,11 +69,18 @@ export function computeAxisTicksDimensions(
if (!scale) {
throw new Error(`Cannot compute scale for axis spec ${axisSpec.id}`);
}

const tickLabelPadding =
axisSpec.style && axisSpec.style.tickLabelPadding
? axisSpec.style.tickLabelPadding
: axisConfig.tickLabelStyle.padding;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we rename this parameter name to themeAxisConfig to make it clearer where this is coming from? I think eventually we'll want to think about how to better merge spec-specific and theme-general styles but for now, would be nice to update the parameter name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for helping me with this! The commit with the changes in naming and improving axis_utils test is addressed in commit d8dd8f4


const dimensions = computeTickDimensions(
scale,
axisSpec.tickFormat,
bboxCalculator,
axisConfig,
tickLabelPadding,
axisSpec.tickLabelRotation,
);

Expand Down Expand Up @@ -133,6 +140,7 @@ export const getMaxBboxDimensions = (
fontSize: number,
fontFamily: string,
tickLabelRotation: number,
tickLabelPadding: number,
) => (
acc: { [key: string]: number },
tickLabel: string,
Expand All @@ -142,7 +150,7 @@ export const getMaxBboxDimensions = (
maxLabelTextWidth: number;
maxLabelTextHeight: number;
} => {
const bbox = bboxCalculator.compute(tickLabel, fontSize, fontFamily).getOrElse({
const bbox = bboxCalculator.compute(tickLabel, tickLabelPadding, fontSize, fontFamily).getOrElse({
width: 0,
height: 0,
});
Expand All @@ -158,7 +166,6 @@ export const getMaxBboxDimensions = (
const prevHeight = acc.maxLabelBboxHeight;
const prevLabelWidth = acc.maxLabelTextWidth;
const prevLabelHeight = acc.maxLabelTextHeight;

return {
maxLabelBboxWidth: prevWidth > width ? prevWidth : width,
maxLabelBboxHeight: prevHeight > height ? prevHeight : height,
Expand All @@ -172,6 +179,7 @@ function computeTickDimensions(
tickFormat: TickFormatter,
bboxCalculator: BBoxCalculator,
axisConfig: AxisConfig,
tickLabelPadding: number,
tickLabelRotation: number = 0,
) {
const tickValues = scale.ticks();
Expand All @@ -182,7 +190,7 @@ function computeTickDimensions(
} = axisConfig;

const { maxLabelBboxWidth, maxLabelBboxHeight, maxLabelTextWidth, maxLabelTextHeight } = tickLabels.reduce(
getMaxBboxDimensions(bboxCalculator, fontSize, fontFamily, tickLabelRotation),
getMaxBboxDimensions(bboxCalculator, fontSize, fontFamily, tickLabelRotation, tickLabelPadding),
{ maxLabelBboxWidth: 0, maxLabelBboxHeight: 0, maxLabelTextWidth: 0, maxLabelTextHeight: 0 },
);

Expand Down
2 changes: 1 addition & 1 deletion src/lib/axes/bbox_calculator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,6 @@ export interface BBox {
}

export interface BBoxCalculator {
compute(text: string, fontSize?: number, fontFamily?: string): Option<BBox>;
compute(text: string, padding: number, fontSize?: number, fontFamily?: string): Option<BBox>;
destroy(): void;
}
14 changes: 7 additions & 7 deletions src/lib/axes/canvas_text_bbox_calculator.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,20 +4,20 @@ import { CanvasTextBBoxCalculator } from './canvas_text_bbox_calculator';
describe('CanvasTextBBoxCalculator', () => {
test('can create a canvas for computing text measurement values', () => {
const canvasBboxCalculator = new CanvasTextBBoxCalculator();
const bbox = canvasBboxCalculator.compute('foo').getOrElse({
const bbox = canvasBboxCalculator.compute('foo', 0).getOrElse({
width: 0,
height: 0,
});
expect(Math.abs(bbox.width - 23.2)).toBeLessThanOrEqual(2);
expect(bbox.height).toBe(16);

canvasBboxCalculator.context = null;
expect(canvasBboxCalculator.compute('foo')).toBe(none);
expect(canvasBboxCalculator.compute('foo', 0)).toBe(none);
});
test('can compute near the same width for the same text independently of the scale factor', () => {
let canvasBboxCalculator = new CanvasTextBBoxCalculator(undefined, 5);

let bbox = canvasBboxCalculator.compute('foo').getOrElse({
let bbox = canvasBboxCalculator.compute('foo', 0).getOrElse({
width: 0,
height: 0,
});
Expand All @@ -26,7 +26,7 @@ describe('CanvasTextBBoxCalculator', () => {

canvasBboxCalculator = new CanvasTextBBoxCalculator(undefined, 10);

bbox = canvasBboxCalculator.compute('foo').getOrElse({
bbox = canvasBboxCalculator.compute('foo', 0).getOrElse({
width: 0,
height: 0,
});
Expand All @@ -35,7 +35,7 @@ describe('CanvasTextBBoxCalculator', () => {

canvasBboxCalculator = new CanvasTextBBoxCalculator(undefined, 50);

bbox = canvasBboxCalculator.compute('foo').getOrElse({
bbox = canvasBboxCalculator.compute('foo', 0).getOrElse({
width: 0,
height: 0,
});
Expand All @@ -44,7 +44,7 @@ describe('CanvasTextBBoxCalculator', () => {

canvasBboxCalculator = new CanvasTextBBoxCalculator(undefined, 100);

bbox = canvasBboxCalculator.compute('foo').getOrElse({
bbox = canvasBboxCalculator.compute('foo', 0).getOrElse({
width: 0,
height: 0,
});
Expand All @@ -53,7 +53,7 @@ describe('CanvasTextBBoxCalculator', () => {

canvasBboxCalculator = new CanvasTextBBoxCalculator(undefined, 1000);

bbox = canvasBboxCalculator.compute('foo').getOrElse({
bbox = canvasBboxCalculator.compute('foo', 0).getOrElse({
width: 0,
height: 0,
});
Expand Down
7 changes: 6 additions & 1 deletion src/lib/axes/canvas_text_bbox_calculator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,16 @@ export class CanvasTextBBoxCalculator implements BBoxCalculator {
this.attachedRoot.appendChild(this.offscreenCanvas);
this.scaledFontSize = scaledFontSize;
}
compute(text: string, fontSize = 16, fontFamily = 'Arial', padding: number = 1): Option<BBox> {
compute(text: string, padding: number, fontSize = 16, fontFamily = 'Arial'): Option<BBox> {
Copy link
Contributor

Choose a reason for hiding this comment

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

@markov00 What do you think about doing some validation to prevent negative padding and if the supplied padding argumen is less than 1, set the padding to 1 instead? And we could also surface this in our configuration warnings once we have those. We are already accounting for padding in the computation of the label and if the user specifies a negative padding, then the text box would not be wide enough for the label.

Copy link
Member

Choose a reason for hiding this comment

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

Yes I'm ok to add some validation here to prevent negative padding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added commit edb1f4b to address

Copy link
Contributor Author

Choose a reason for hiding this comment

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

test added in 1507d9c to address

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to confirm - should we set the padding to 1 when it's specified by the user at 0? Or should the validation only catch negative values? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clarified in zoom with Emma about this 👌

if (!this.context) {
return none;
}

// Avoid having negative padding that can obscure text
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd say generally comments as simple as this can be left for tests. Meaning you should create test cases that validate this line of code. That way if someone removes these lines of code, the tests break.

Not saying you can't leave comments, but do so for more complex code that requires explanation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok! I think I wrote one that gets to the issue in commit 1507d9c. Let me know what you think thanks!

Copy link
Collaborator

Choose a reason for hiding this comment

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

FYI Github will auto link and format the commit hash when you just put the hash or short hash like this 1507d9c but when you wrap the hash in backticks, or anything else for that matter, it doesn't create the link like this 1507d9c. Here is a link to all the url fun https://help.github.com/en/articles/autolinked-references-and-urls

You can also paste code snippets by selecting code lines, use shift+click for multiple lines, and select copy permalink

image

Then you will get something like this...

"scripts": {
"cz": "git-cz",
"build:clean": "rm -rf ./dist",

Just FYI - It took me a while to realize how many helpful tools GH has for our use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

improved comment in commit d8dd8f4

if (padding < 1) {
padding = 1;
}

// We scale the text up to get a more accurate computation of the width of the text
// because `measureText` can vary a lot between browsers.
const scalingFactor = this.scaledFontSize / fontSize;
Expand Down
12 changes: 8 additions & 4 deletions src/lib/series/rendering.ts
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,8 @@ export function renderBars(
const barGeometries: BarGeometry[] = [];

const bboxCalculator = new CanvasTextBBoxCalculator();
// default padding to 1 for now
const padding = 1;
const fontSize = seriesStyle && seriesStyle.displayValue ? seriesStyle.displayValue.fontSize : undefined;
const fontFamily = seriesStyle && seriesStyle.displayValue ? seriesStyle.displayValue.fontFamily : undefined;

Expand Down Expand Up @@ -253,10 +255,12 @@ export function renderBars(
: undefined
: formattedDisplayValue;

const computedDisplayValueWidth = bboxCalculator.compute(displayValueText || '', fontSize, fontFamily).getOrElse({
width: 0,
height: 0,
}).width;
const computedDisplayValueWidth = bboxCalculator
.compute(displayValueText || '', padding, fontSize, fontFamily)
.getOrElse({
width: 0,
height: 0,
}).width;
const displayValueWidth =
displayValueSettings && displayValueSettings.isValueContainedInElement ? width : computedDisplayValueWidth;

Expand Down
7 changes: 7 additions & 0 deletions src/lib/series/specs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -218,10 +218,17 @@ export interface AxisSpec {
title?: string;
/** If specified, it constrains the domain for these values */
domain?: DomainRange;
/** Object to hold custom styling */
style?: CustomStyle;
}

export type TickFormatter = (value: any) => string;

interface CustomStyle {
/** Specifies the amount of padding on the tick label bounding box */
tickLabelPadding?: number;
}
markov00 marked this conversation as resolved.
Show resolved Hide resolved

/**
* The position of the axis relative to the chart.
* A left or right positioned axis is a vertical axis.
Expand Down
2 changes: 1 addition & 1 deletion src/lib/themes/dark_theme.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ export const DARK_THEME: Theme = {
fontFamily: 'sans-serif',
fontStyle: 'normal',
fill: 'white',
padding: 0,
padding: 1,
},
tickLineStyle: {
stroke: 'white',
Expand Down
2 changes: 1 addition & 1 deletion src/lib/themes/light_theme.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ export const LIGHT_THEME: Theme = {
fontFamily: 'sans-serif',
fontStyle: 'normal',
fill: 'gray',
padding: 0,
padding: 1,
rshen91 marked this conversation as resolved.
Show resolved Hide resolved
},
tickLineStyle: {
stroke: 'gray',
Expand Down
34 changes: 31 additions & 3 deletions stories/axis.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -54,15 +54,26 @@ function renderAxisWithOptions(position: Position, seriesGroup: string, show: bo

storiesOf('Axis', module)
.add('basic', () => {
const customStyle = {
tickLabelPadding: number('Tick Label Padding', 0),
};

return (
<Chart className={'story-chart'}>
<Settings debug={boolean('debug', false)} />
<Axis id={getAxisId('bottom')} position={Position.Bottom} title={'Bottom axis'} showOverlappingTicks={true} />
<Axis
id={getAxisId('bottom')}
position={Position.Bottom}
title={'Bottom axis'}
showOverlappingTicks={true}
style={customStyle}
/>
<Axis
id={getAxisId('left2')}
title={'Left axis'}
position={Position.Left}
tickFormat={(d) => Number(d).toFixed(2)}
style={customStyle}
/>

<AreaSeries
Expand All @@ -77,6 +88,10 @@ storiesOf('Axis', module)
);
})
.add('tick label rotation', () => {
const customStyle = {
tickLabelPadding: number('Tick Label Padding', 0),
};

return (
<Chart className={'story-chart'}>
<Axis
Expand All @@ -90,6 +105,7 @@ storiesOf('Axis', module)
max: 90,
step: 1,
})}
style={customStyle}
/>
<Axis
id={getAxisId('left')}
Expand All @@ -102,6 +118,7 @@ storiesOf('Axis', module)
step: 1,
})}
tickFormat={(d) => Number(d).toFixed(2)}
style={customStyle}
/>
<Axis
id={getAxisId('top')}
Expand All @@ -114,6 +131,7 @@ storiesOf('Axis', module)
step: 1,
})}
tickFormat={(d) => Number(d).toFixed(2)}
style={customStyle}
/>
<Axis
id={getAxisId('right')}
Expand All @@ -126,6 +144,7 @@ storiesOf('Axis', module)
step: 1,
})}
tickFormat={(d) => Number(d).toFixed(2)}
style={customStyle}
/>
<AreaSeries
id={getSpecId('lines')}
Expand Down Expand Up @@ -310,10 +329,20 @@ storiesOf('Axis', module)
.add('w many tick labels', () => {
const dg = new DataGenerator();
const data = dg.generateSimpleSeries(31);
const customStyle = {
tickLabelPadding: number('Tick Label Padding', 0),
};

return (
<Chart className={'story-chart'}>
<Settings debug={true} />
<Axis id={getAxisId('bottom')} position={Position.Bottom} title={'Bottom axis'} showOverlappingTicks={true} />
<Axis
id={getAxisId('bottom')}
position={Position.Bottom}
title={'Bottom axis'}
showOverlappingTicks={true}
style={customStyle}
/>
<AreaSeries
id={getSpecId('lines')}
xScaleType={ScaleType.Linear}
Expand Down Expand Up @@ -345,7 +374,6 @@ storiesOf('Axis', module)
min: number('xDomain min', 0),
max: number('xDomain max', 3),
};

return (
<Chart className={'story-chart'}>
<Settings showLegend={false} xDomain={xDomain} />
Expand Down
Loading