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

[Elastic Charts] Added value labels styles for bar charts #4845

Merged
merged 22 commits into from
Sep 16, 2021
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
2cd5539
Adding new theme value labels styling
miukimiu Jun 1, 2021
c09bfdf
Merge remote-tracking branch 'upstream/master' into xy-chart-theme-va…
miukimiu Jul 16, 2021
6c4e731
Merge remote-tracking branch 'upstream/master' into xy-chart-theme-va…
miukimiu Jul 26, 2021
e1f3f7c
Setting `textBorder` to zero
miukimiu Jul 26, 2021
9382384
Merge remote-tracking branch 'upstream/master' into xy-chart-theme-va…
miukimiu Jul 26, 2021
56a8aab
Adding `showValueLabels` field in MultiChartCard
miukimiu Jul 26, 2021
7a3f23d
Increase category chart height
miukimiu Jul 27, 2021
20e86ea
Adding center alignment
miukimiu Jul 27, 2021
90166c3
CL
miukimiu Jul 27, 2021
69e1ab3
Shared comp improvement
miukimiu Jul 27, 2021
8d1242f
Update CHANGELOG.md
miukimiu Jul 28, 2021
f63de66
Merge remote-tracking branch 'upstream/master' into xy-chart-theme-va…
miukimiu Jul 28, 2021
7973a88
Adding value labels card and config text to copy.
miukimiu Jul 28, 2021
a5c0094
Merge remote-tracking branch 'upstream/master' into xy-chart-theme-va…
miukimiu Jul 28, 2021
e82929f
Merge remote-tracking branch 'upstream/master' into xy-chart-theme-va…
miukimiu Sep 9, 2021
9ea28f2
Adding multi and stacked centered
miukimiu Sep 9, 2021
924eaf8
CL
miukimiu Sep 9, 2021
91e91f5
Adding better data for multi series and removing lodash orderBy
miukimiu Sep 9, 2021
d3c215c
Better indentation
miukimiu Sep 9, 2021
2833783
Merge remote-tracking branch 'upstream/master' into xy-chart-theme-va…
miukimiu Sep 9, 2021
9a19371
Custom theme only when value labels. Small styles fixes.
miukimiu Sep 16, 2021
467370e
Merge remote-tracking branch 'upstream/master' into xy-chart-theme-va…
miukimiu Sep 16, 2021
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
- Refactored styles of `EuiTabs` ([#5135](https://github.com/elastic/eui/pull/5135))
- Removed Sass variables for `EuiTabs` font size (`$euiTabFontSize, $euiTabFontSizeS, $euiTabFontSizeL`) ([#5135](https://github.com/elastic/eui/pull/5135))
- Extended all `EuiTabProps` for each `EuiTabbedContentTab` ([#5135](https://github.com/elastic/eui/pull/5135))
- Updated `barSeriesStyle.displayValue` of the elastic-charts `Theme` for better default styles ([#4845](https://github.com/elastic/eui/pull/4845))
miukimiu marked this conversation as resolved.
Show resolved Hide resolved
- Added `useGeneratedHtmlId` utility, which memoizes the randomly generated ID on mount and prevents regenerated IDs on component rerender ([#5133](https://github.com/elastic/eui/pull/5133))

**Theme: Amsterdam**
Expand Down
167 changes: 131 additions & 36 deletions src-docs/src/views/elastic_charts/category_chart.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ export const CategoryChart = () => {
const [ordered, setOrdered] = useState(true);
const [formatted, setFormatted] = useState(false);
const [chartType, setChartType] = useState('BarSeries');

const [valueLabels, setValueLabels] = useState(false);
const onMultiChange = (multiObject) => {
const { multi, stacked } = multiObject;
setMulti(multi);
Expand All @@ -61,6 +61,10 @@ export const CategoryChart = () => {
setChartType(chartType);
};

const onValueLabelsChange = (e) => {
setValueLabels(e.target.checked);
};

const isDarkTheme = themeContext.theme.includes('dark');
const theme = isDarkTheme
? EUI_CHARTS_THEME_DARK.theme
Expand All @@ -70,6 +74,114 @@ export const CategoryChart = () => {

const DATASET = multi ? GITHUB_DATASET : SIMPLE_GITHUB_DATASET;

const displayValueSettings = {
showValueLabel: true,
};

const customTheme = {
...theme,
barSeriesStyle: {
displayValue: {
...theme.barSeriesStyle.displayValue,
offsetX: rotated ? 2 : 0,
offsetY: rotated ? 0 : -2,
Copy link
Member

Choose a reason for hiding this comment

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

From what I see, this is used as padding on the text. We can probably introduce a default or custom padding and we can get rid of this part

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn 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.

I agree 👍🏽

Copy link
Contributor

Choose a reason for hiding this comment

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

Optional, but I'd love to see even more separation.

Suggested change
offsetX: rotated ? 2 : 0,
offsetY: rotated ? 0 : -2,
offsetX: rotated ? 4 : 0,
offsetY: rotated ? 0 : -4,

...(multi && stacked
? {
alignment: {
vertical: 'middle',
horizontal: 'center',
},
}
: {
alignment: rotated
? {
vertical: 'middle',
}
: {
horizontal: 'center',
},
}),
},
},
};

const defaultAlignmentToCopy = `alignment: {
vertical: 'middle',
horizontal: 'center',
}`;

const alignmentRotatedToCopy = rotated
? `alignment: {
vertical: 'middle',
}`
: `alignment: {
horizontal: 'center',
}`;

const chartVariablesToCopy = `const theme = isDarkTheme
? EUI_CHARTS_THEME_DARK.theme
: EUI_CHARTS_THEME_LIGHT.theme;

const customTheme = {
...theme,
barSeriesStyle: {
displayValue: {
...theme.barSeriesStyle.displayValue,
offsetX: ${rotated ? '2' : '0'},
offsetY: ${rotated ? '0' : '-2'},
${multi && stacked ? defaultAlignmentToCopy : alignmentRotatedToCopy},
},
},
};`;
miukimiu marked this conversation as resolved.
Show resolved Hide resolved

const multiConfigData = `[{ vizType: "Data Table", count: 6, issueType: "Bug" },
{ vizType: "Data Table", count: 24, issueType: "Other" },
{ vizType: "Heatmap", count: 12, issueType: "Bug" },
{ vizType: "Heatmap", count: 20, issueType: "Other" }]
`;

const singleConfigData = `[{vizType: 'Data Table', count: 24, issueType: 'Bug'},
{vizType: 'Heatmap',count: 12, issueType: 'Other'}]
`;

const chartConfigurationToCopy = `<Chart size={{height: 300}}>
<Settings
theme={customTheme}
rotation={${rotated ? 90 : 0}}
showLegend={${multi}}
${multi ? 'legendPosition="right"' : ''}
/>
<${chartType}
id="issues"
name="Issues"
data={
${multi ? multiConfigData : singleConfigData}
}
xAccessor="vizType"
yAccessors={['count']}
${multi ? "splitSeriesAccessors={['issueType']}" : ''}
${stacked ? "stackAccessors={['issueType']}" : ''}
${valueLabels ? 'displayValueSettings={{showValueLabel: true}}' : ''}
/>
<Axis
id="bottom-axis"
position={${rotated ? '"left"' : '"bottom"'}}
showGridLines={false}
/>
<Axis
id="left-axis"
position={${rotated ? '"bottom"' : '"left"'}}
${formatted ? 'tickFormat={d => `${round(Number(d) / 1000, 2)}k`}' : ''}
Copy link
Contributor

Choose a reason for hiding this comment

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

@markov00 The one thing I noticed is that when we format the tick labels on a particular axis, it does also get applied to the showValueLabel setting, except when the chart gets rotated. Is this a known issue in elastic-charts?

Screen Shot 2021-09-14 at 11 45 57 AM

Copy link
Member

Choose a reason for hiding this comment

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

thank you @cchaos you are right, this is an issue in our code. I will check and fix the bug soon.

Copy link
Member

Choose a reason for hiding this comment

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

/>
</Chart>`;

const removeEmptyLines = (string) => string.replace(/(^[ \t]*\n)/gm, '');

const textToCopy = `${chartVariablesToCopy}

${removeEmptyLines(chartConfigurationToCopy)}
`;
miukimiu marked this conversation as resolved.
Show resolved Hide resolved

return (
<Fragment>
<EuiTitle size="xxs">
Expand All @@ -81,9 +193,9 @@ export const CategoryChart = () => {

<EuiSpacer size="s" />

<Chart size={{ height: 300 }}>
<Chart size={{ height: 400 }}>
<Settings
theme={theme}
theme={customTheme}
miukimiu marked this conversation as resolved.
Show resolved Hide resolved
showLegend={multi}
legendPosition="right"
rotation={rotated ? 90 : 0}
Expand All @@ -100,6 +212,7 @@ export const CategoryChart = () => {
yAccessors={['count']}
splitSeriesAccessors={multi ? ['issueType'] : undefined}
stackAccessors={stacked ? ['issueType'] : undefined}
displayValueSettings={valueLabels && displayValueSettings}
/>
<Axis
id="bottom-axis"
Expand Down Expand Up @@ -170,44 +283,26 @@ export const CategoryChart = () => {
<EuiFlexItem>
<MultiChartCard onChange={onMultiChange} />
</EuiFlexItem>

<EuiFlexItem>
<ChartCard
title="Value labels"
description="Value labels can add too much detail and make categorical charts more difficult to interpret. Consider showing them only when the values are of extreme importance."
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: usually the reason why you want to show numbers on a chart is that you want to clearly present the exact number (all digits/decimal digits are important). If that is the case, a suggestion could be to include, on the side, a small table that definitely shows clearly all the numbers and allow the user to sort them by value or categories.

>
<EuiSpacer size="s" />
<EuiSwitch
label="Show value labels"
checked={valueLabels}
onChange={onValueLabelsChange}
/>
</ChartCard>
</EuiFlexItem>
</EuiFlexGrid>

<EuiSpacer />

<div className="eui-textCenter">
<EuiCopy
textToCopy={`<Chart size={{height: 300}}>
<Settings
theme={isDarkTheme ? EUI_CHARTS_THEME_DARK.theme : EUI_CHARTS_THEME_LIGHT.theme}
rotation={${rotated ? 90 : 0}}
showLegend={${multi}}
${multi ? 'legendPosition="right"' : ''}
/>
<${chartType}
id="issues"
name="Issues"
data={${
ordered
? "orderBy([{vizType: 'Data Table', count: 24, issueType: 'Bug'},{vizType: 'Heatmap',count: 12, issueType: 'Other'}], ['count'], ['desc'])"
: "orderBy([{vizType: 'Data Table', count: 24, issueType: 'Bug'},{vizType: 'Heatmap',count: 12, issueType: 'Other'}], ['vizType'], ['asc'])"
}}
xAccessor="vizType"
yAccessors={['count']}
${multi ? "splitSeriesAccessors={['issueType']}" : ''}
${stacked ? "stackAccessors={['issueType']}" : ''}
/>
<Axis
id="bottom-axis"
position={${rotated ? 'left' : 'bottom'}}
showGridLines={false}
/>
<Axis
id="left-axis"
position={${rotated ? 'bottom' : 'left'}}
${formatted ? 'tickFormat={d => `${round(Number(d) / 1000, 2)}k`}' : ''}
/>
</Chart>`}
>
<EuiCopy textToCopy={textToCopy}>
{(copy) => (
<EuiButton fill onClick={copy} iconType="copyClipboard">
Copy code of current configuration
Expand Down
12 changes: 10 additions & 2 deletions src/themes/charts/themes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,9 +100,17 @@ function createTheme(colors: any): EuiChartThemeType {
},
barSeriesStyle: {
displayValue: {
fontSize: 8,
fontSize: 10,
Copy link
Member

Choose a reason for hiding this comment

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

You can also set a min and max font size, the code automatically uses the right size depending on the available space.

fontFamily: fontFamily,
fill: colors.euiTextSubduedColor.rgba,
fill: {
textInvertible: true,
textContrast: true,
textBorder: 0,
},
alignment: {
horizontal: 'center',
vertical: 'middle',
},
},
},
scales: {
Expand Down