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
Changes from 1 commit
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
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
@@ -35,6 +35,7 @@ export function renderText(
},
degree: number = 0,
translation?: Partial<Point>,
scale: number = 1,
) {
if (text === undefined || text === null) {
return;
@@ -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);
});
});
}
@@ -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;
}
12 changes: 7 additions & 5 deletions src/chart_types/xy_chart/renderer/canvas/values/bar.ts
Original file line number Diff line number Diff line change
@@ -45,7 +45,7 @@ export function renderBarValues(ctx: CanvasRenderingContext2D, props: BarValuesP
if (!displayValue) {
continue;
}
const { text, fontSize } = displayValue;
const { text, fontSize, fontScale } = displayValue;
let textLines = {
lines: [text],
width: displayValue.width,
@@ -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,
@@ -95,6 +95,8 @@ export function renderBarValues(ctx: CanvasRenderingContext2D, props: BarValuesP
baseline,
},
-chartRotation,
undefined,
fontScale,
);
}
}
94 changes: 76 additions & 18 deletions src/chart_types/xy_chart/rendering/rendering.ts
Original file line number Diff line number Diff line change
@@ -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';
@@ -56,6 +57,59 @@ 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;
return finalFontSize <= limits.max && finalFontSize >= limits.min ? scale : 0;
dej611 marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* 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 = typeof fontSize === 'number' ? fontSize : fontSize.min;

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
@@ -306,6 +360,7 @@ export function renderBars(
styleAccessor?: BarStyleAccessor,
minBarHeight?: number,
stackMode?: StackMode,
chartRotation?: number,
): {
barGeometries: BarGeometry[];
indexedGeometryMap: IndexedGeometryMap;
@@ -384,34 +439,37 @@ 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 targetFontSize = displayValueText
? Math.max(Math.floor(width / displayValueText.length), fontSize)
: fontSize;

const computedDisplayValueWidth = bboxCalculator.compute(
const { displayValueWidth, fixedFontScale } = computeBoxWidth(
displayValueText || '',
padding,
targetFontSize,
fontFamily,
).width;
const displayValueWidth =
displayValueSettings && displayValueSettings.isValueContainedInElement ? width : computedDisplayValueWidth;
{ 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 = isHorizontalRotation ? displayValueWidth : fixedFontScale;

const tempTextScalingFactor = (width * fontSizeFactor) / referenceWidth;
markov00 marked this conversation as resolved.
Show resolved Hide resolved
const textScalingFactor = getFinalFontScalingFactor(tempTextScalingFactor, 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
? {
fontSize: targetFontSize,
fontScale: textScalingFactor,
fontSize: fixedFontScale,
text: displayValueText,
width: displayValueWidth,
height: fontSize,
width: bboxWidthFactor * displayValueWidth,
height: textScalingFactor * fixedFontScale,
hideClippedValue,
isValueContainedInElement: displayValueSettings.isValueContainedInElement,
}
7 changes: 6 additions & 1 deletion src/chart_types/xy_chart/state/utils/utils.ts
Original file line number Diff line number Diff line change
@@ -324,6 +324,7 @@ export function computeSeriesGeometries(
chartTheme,
enableHistogramMode,
stackMode,
chartRotation,
);
orderIndex = counts[SeriesTypes.Bar] > 0 ? orderIndex + 1 : orderIndex;
areas.push(...geometries.areas);
@@ -363,6 +364,8 @@ export function computeSeriesGeometries(
axesSpecs,
chartTheme,
enableHistogramMode,
undefined,
chartRotation,
);
orderIndex = counts[SeriesTypes.Bar] > 0 ? orderIndex + counts[SeriesTypes.Bar] : orderIndex;

@@ -464,7 +467,8 @@ function renderGeometries(
axesSpecs: AxisSpec[],
chartTheme: Theme,
enableHistogramMode: boolean,
stackMode?: StackMode,
stackMode: StackMode | undefined,
chartRotation: number,
markov00 marked this conversation as resolved.
Show resolved Hide resolved
): {
points: PointGeometry[];
bars: BarGeometry[];
@@ -527,6 +531,7 @@ function renderGeometries(
spec.styleAccessor,
spec.minBarHeight,
stackMode,
chartRotation,
);
indexedGeometryMap.merge(renderedBars.indexedGeometryMap);
bars.push(...renderedBars.barGeometries);
1 change: 1 addition & 0 deletions src/utils/geometry.ts
Original file line number Diff line number Diff line change
@@ -69,6 +69,7 @@ export interface BarGeometry {
height: number;
color: Color;
displayValue?: {
fontScale?: number;
fontSize: number;
text: any;
width: number;
9 changes: 8 additions & 1 deletion src/utils/themes/theme.ts
Original file line number Diff line number Diff line change
@@ -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;
};
7 changes: 6 additions & 1 deletion stories/bar/50_simple_value_label.tsx
Original file line number Diff line number Diff line change
@@ -53,11 +53,16 @@ export const Example = () => {
};

const debug = boolean('debug', false);
const fixedFontSize = number('Fixed font size', 10);
const useFixedFontSize = boolean('Use fixed font size', false);

const maxFontSize = number('Max font size', 25);
const minFontSize = number('Min font size', 10);

const theme = {
barSeriesStyle: {
displayValue: {
fontSize: number('value font size', 10),
fontSize: useFixedFontSize ? fixedFontSize : { max: maxFontSize, min: minFontSize },
fontFamily: "'Open Sans', Helvetica, Arial, sans-serif",
fontStyle: 'normal',
padding: 0,
markov00 marked this conversation as resolved.
Show resolved Hide resolved