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(bar_chart): scaled font size for value labels #789

Merged
merged 15 commits into from
Oct 14, 2020
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
7 changes: 6 additions & 1 deletion api/charts.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -535,7 +535,12 @@ export interface DisplayValueSpec {
}

// @public (undocumented)
export type DisplayValueStyle = TextStyle & {
export type DisplayValueStyle = Omit<TextStyle, 'fontSize'> & {
fontSize: number | {
min: number;
max: number;
};
} & {
offsetX: number;
offsetY: number;
};
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
11 changes: 7 additions & 4 deletions src/chart_types/xy_chart/renderer/canvas/primitives/text.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ export function renderText(
},
degree: number = 0,
translation?: Partial<Point>,
scale: number = 1,
) {
if (text === undefined || text === null) {
return;
Expand All @@ -49,7 +50,9 @@ export function renderText(
if (translation?.x || translation?.y) {
ctx.translate(translation?.x ?? 0, translation?.y ?? 0);
}
ctx.fillText(text, origin.x, origin.y);
ctx.translate(origin.x, origin.y);
ctx.scale(scale, scale);
ctx.fillText(text, 0, 0);
});
});
}
Expand Down Expand Up @@ -81,14 +84,14 @@ export function wrapLines(
const shouldAddEllipsis = false;
const textArr: string[] = [];
const textMeasureProcessor = measureText(ctx);
const getTextWidth = (text: string) => {
const getTextWidth = (textString: string) => {
const measuredText = textMeasureProcessor(fontSize, [
{
text,
text: textString,
...font,
},
]);
const measure = measuredText[0];
const [measure] = measuredText;
if (measure) {
return measure.width;
}
Expand Down
14 changes: 8 additions & 6 deletions src/chart_types/xy_chart/renderer/canvas/values/bar.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,14 +38,14 @@ interface BarValuesProps {
/** @internal */
export function renderBarValues(ctx: CanvasRenderingContext2D, props: BarValuesProps) {
const { bars, debug, chartRotation, chartDimensions, theme } = props;
const { fontFamily, fontStyle, fill, fontSize } = theme.barSeriesStyle.displayValue;
const { fontFamily, fontStyle, fill } = theme.barSeriesStyle.displayValue;
const barsLength = bars.length;
for (let i = 0; i < barsLength; i++) {
const { displayValue } = bars[i];
if (!displayValue) {
continue;
}
const { text } = displayValue;
const { text, fontSize, fontScale } = displayValue;
let textLines = {
lines: [text],
width: displayValue.width,
Expand Down Expand Up @@ -80,13 +80,13 @@ export function renderBarValues(ctx: CanvasRenderingContext2D, props: BarValuesP
const { width, height } = textLines;
const linesLength = textLines.lines.length;

for (let i = 0; i < linesLength; i++) {
const text = textLines.lines[i];
const origin = repositionTextLine({ x, y }, chartRotation, i, linesLength, { height, width });
for (let j = 0; j < linesLength; j++) {
const textLine = textLines.lines[j];
const origin = repositionTextLine({ x, y }, chartRotation, j, linesLength, { height, width });
renderText(
ctx,
origin,
text,
textLine,
{
...font,
fill,
Expand All @@ -95,6 +95,8 @@ export function renderBarValues(ctx: CanvasRenderingContext2D, props: BarValuesP
baseline,
},
-chartRotation,
undefined,
fontScale,
);
}
}
Expand Down
97 changes: 87 additions & 10 deletions src/chart_types/xy_chart/rendering/rendering.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ import {
GeometryStateStyle,
LineStyle,
BubbleSeriesStyle,
DisplayValueStyle,
} from '../../../utils/themes/theme';
import { IndexedGeometryMap, GeometryType } from '../utils/indexed_geometry_map';
import { DataSeriesDatum, DataSeries, XYChartSeriesIdentifier } from '../utils/series';
Expand All @@ -56,6 +57,66 @@ export interface MarkSizeOptions {
enabled: boolean;
ratio?: number;
}
/**
* Returns a safe scaling factor for label text for fixed or range size inputs
* @internal
*/
function getFinalFontScalingFactor(
scale: number,
fixedFontSize: number,
limits: DisplayValueStyle['fontSize'],
): number {
if (typeof limits === 'number') {
// it's a fixed size, so it's always ok
return 1;
}
const finalFontSize = scale * fixedFontSize;
if (finalFontSize > limits.max) {
return limits.max / fixedFontSize;
markov00 marked this conversation as resolved.
Show resolved Hide resolved
}
if (finalFontSize < limits.min) {
// it's technically 1, but keep it generic in case the fixedFontSize changes
return limits.min / fixedFontSize;
markov00 marked this conversation as resolved.
Show resolved Hide resolved
}
return scale;
}

/**
* Workout the text box size and fixedFontSize based on a collection of options
* @internal
*/
function computeBoxWidth(
text: string,
{
padding,
fontSize,
fontFamily,
bboxCalculator,
width,
}: {
padding: number;
fontSize: number | { min: number; max: number };
fontFamily: string;
bboxCalculator: CanvasTextBBoxCalculator;
width: number;
},
displayValueSettings: DisplayValueSpec | undefined,
): { fixedFontScale: number; displayValueWidth: number } {
const fixedFontScale = Math.max(typeof fontSize === 'number' ? fontSize : fontSize.min, 1);

const computedDisplayValueWidth = bboxCalculator.compute(text || '', padding, fixedFontScale, fontFamily).width;
if (typeof fontSize !== 'number') {
markov00 marked this conversation as resolved.
Show resolved Hide resolved
return {
fixedFontScale,
displayValueWidth: computedDisplayValueWidth,
};
}
return {
fixedFontScale,
displayValueWidth:
displayValueSettings && displayValueSettings.isValueContainedInElement ? width : computedDisplayValueWidth,
};
}

/**
* Returns value of `y1` or `filled.y1` or null
Expand Down Expand Up @@ -306,6 +367,7 @@ export function renderBars(
styleAccessor?: BarStyleAccessor,
minBarHeight?: number,
stackMode?: StackMode,
chartRotation?: number,
): {
barGeometries: BarGeometry[];
indexedGeometryMap: IndexedGeometryMap;
Expand Down Expand Up @@ -384,25 +446,40 @@ export function renderBars(

// only show displayValue for even bars if showOverlappingValue
const displayValueText =
displayValueSettings && displayValueSettings.isAlternatingValueLabel
? barGeometries.length % 2 === 0
? formattedDisplayValue
: undefined
displayValueSettings && displayValueSettings.isAlternatingValueLabel && barGeometries.length % 2
? undefined
: formattedDisplayValue;

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

const isHorizontalRotation = chartRotation == null || [0, 180].includes(chartRotation);
Copy link
Author

Choose a reason for hiding this comment

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

Either the logic or naming is wrong, the horizontal charts are 90 and 270: https://elastic.github.io/elastic-charts/?path=/story/rotations--rotations-90-deg-ordinal

Copy link
Contributor

Choose a reason for hiding this comment

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

Eheh I had a chat with @markov00 about that the other day. He explained to me that their naming reference where the bars are located (for a vertical bar chart they are located on the x axis, therefore horizontal).
I used isVertical here before which makes more sense for me as well, then changed after the chat as consitency is best when maintain the codebase.

// Take 70% of space for the label text
const fontSizeFactor = 0.7;
// Pick the right side of the label's box to use as factor reference
const referenceWidth = Math.max(isHorizontalRotation ? displayValueWidth : fixedFontScale, 1);

const textScalingFactor = getFinalFontScalingFactor(
(width * fontSizeFactor) / referenceWidth,
fixedFontScale,
fontSize,
);

const hideClippedValue = displayValueSettings ? displayValueSettings.hideClippedValue : undefined;
// Based on rotation scale the width of the text box
const bboxWidthFactor = isHorizontalRotation ? textScalingFactor : 1;

const displayValue =
displayValueSettings && displayValueSettings.showValueLabel
? {
fontScale: textScalingFactor,
fontSize: fixedFontScale,
text: displayValueText,
width: displayValueWidth,
height: fontSize,
width: bboxWidthFactor * displayValueWidth,
height: textScalingFactor * fixedFontScale,
hideClippedValue,
isValueContainedInElement: displayValueSettings.isValueContainedInElement,
}
Expand Down
4 changes: 4 additions & 0 deletions src/chart_types/xy_chart/state/utils/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,7 @@ export function computeSeriesGeometries(
axesSpecs,
chartTheme,
enableHistogramMode,
chartRotation,
stackMode,
);
orderIndex = counts[SeriesTypes.Bar] > 0 ? orderIndex + 1 : orderIndex;
Expand Down Expand Up @@ -363,6 +364,7 @@ export function computeSeriesGeometries(
axesSpecs,
chartTheme,
enableHistogramMode,
chartRotation,
);
orderIndex = counts[SeriesTypes.Bar] > 0 ? orderIndex + counts[SeriesTypes.Bar] : orderIndex;

Expand Down Expand Up @@ -464,6 +466,7 @@ function renderGeometries(
axesSpecs: AxisSpec[],
chartTheme: Theme,
enableHistogramMode: boolean,
chartRotation: number,
stackMode?: StackMode,
): {
points: PointGeometry[];
Expand Down Expand Up @@ -527,6 +530,7 @@ function renderGeometries(
spec.styleAccessor,
spec.minBarHeight,
stackMode,
chartRotation,
);
indexedGeometryMap.merge(renderedBars.indexedGeometryMap);
bars.push(...renderedBars.barGeometries);
Expand Down
1 change: 1 addition & 0 deletions src/mocks/geometries.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ export class MockBarGeometry {
height: 0,
color,
displayValue: {
fontSize: 10,
text: '',
width: 0,
height: 0,
Expand Down
2 changes: 2 additions & 0 deletions src/utils/geometry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,8 @@ export interface BarGeometry {
height: number;
color: Color;
displayValue?: {
fontScale?: number;
fontSize: number;
text: any;
width: number;
height: number;
Expand Down
9 changes: 8 additions & 1 deletion src/utils/themes/theme.ts
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,14 @@ export interface Theme {
export type PartialTheme = RecursivePartial<Theme>;

/** @public */
export type DisplayValueStyle = TextStyle & {
export type DisplayValueStyle = Omit<TextStyle, 'fontSize'> & {
fontSize:
| number
| {
min: number;
max: number;
};
} & {
offsetX: number;
offsetY: number;
};
Expand Down
Loading