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

Upgrade elastic charts v43.1.1 #121593

Merged
merged 38 commits into from
Jan 27, 2022
Merged
Show file tree
Hide file tree
Changes from 33 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
2b95c18
bump version
nickofthyme Dec 18, 2021
ab60941
fix breaking changes in partition charts
nickofthyme Dec 18, 2021
40c513d
fix breaking changes in heatmap charts
nickofthyme Dec 18, 2021
d6e4973
fix more type errors
nickofthyme Dec 18, 2021
76ac2f3
Merge branch 'main' into upgrade-charts-41-breaking
nickofthyme Jan 4, 2022
b9071a4
use workaround for eui blocking changes
nickofthyme Jan 4, 2022
da31c8e
fix pr check errors
nickofthyme Jan 5, 2022
1d1ac03
fix pr check errors
nickofthyme Jan 5, 2022
68f6d69
attempt fn test failure
nickofthyme Jan 6, 2022
a4c609d
Merge branch 'main' into upgrade-charts-41-breaking
nickofthyme Jan 6, 2022
f02912f
fix failed binding on heatmap convert formatter
nickofthyme Jan 6, 2022
bf3e4e4
Merge branch 'main' into upgrade-charts-41-breaking
kibanamachine Jan 10, 2022
74b1118
fix breaking type changes from 42.0.0
nickofthyme Jan 11, 2022
740c0ee
Merge branch 'main' into upgrade-charts-41-breaking
nickofthyme Jan 11, 2022
bf4d5ac
update jest snapshot from spec component changes
nickofthyme Jan 12, 2022
b0b14b4
Merge branch 'main' into upgrade-charts-41-breaking
nickofthyme Jan 12, 2022
97a7bdb
fix failing jest snapshots
nickofthyme Jan 12, 2022
b2e65ba
Merge branch 'main' into upgrade-charts-41-breaking
nickofthyme Jan 13, 2022
08dffcd
fix another jest snapshot
nickofthyme Jan 13, 2022
3e8e1e1
Fix partition inner size
markov00 Jan 14, 2022
62b7a02
upgrade charts to fix heatmap issues
nickofthyme Jan 14, 2022
82b7c9c
Merge branch 'main' into upgrade-charts-41-breaking
nickofthyme Jan 14, 2022
2ad9966
fix package.json
nickofthyme Jan 14, 2022
db668f9
Merge branch 'main' into upgrade-charts-41-breaking
kibanamachine Jan 17, 2022
cade7ea
Merge branch 'main' into upgrade-charts-41-breaking
nickofthyme Jan 17, 2022
b1406f5
fix changes apm chart styles
nickofthyme Jan 17, 2022
20fca8c
update new change on snapshots
nickofthyme Jan 18, 2022
2dd35e4
Merge branch 'main' into upgrade-charts-41-breaking
kibanamachine Jan 20, 2022
d072367
Merge branch 'main' into upgrade-charts-41-breaking
nickofthyme Jan 20, 2022
98597ab
fix issue with heatmap axes and legend labels
nickofthyme Jan 20, 2022
81d7707
add ResizeObserver pollyfill to jest setup
nickofthyme Jan 21, 2022
1418ec5
Merge branch 'main' into upgrade-charts-41-breaking
kibanamachine Jan 21, 2022
a6534d1
Merge branch 'main' into upgrade-charts-41-breaking
kibanamachine Jan 24, 2022
e10b732
Revert adding resize observer polyfill
markov00 Jan 24, 2022
f48385c
Merge remote-tracking branch 'upstream/main' into pr/121593
markov00 Jan 25, 2022
417c401
Update charts to 43.1.0
markov00 Jan 25, 2022
7daab93
Revert removal of resize-observer-polyfill
markov00 Jan 26, 2022
68e6570
Merge remote-tracking branch 'upstream/main' into pr/121593
markov00 Jan 26, 2022
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
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@
"@elastic/apm-rum": "^5.10.1",
"@elastic/apm-rum-react": "^1.3.3",
"@elastic/apm-synthtrace": "link:bazel-bin/packages/elastic-apm-synthtrace",
"@elastic/charts": "40.2.0",
"@elastic/charts": "43.0.1",
"@elastic/datemath": "link:bazel-bin/packages/elastic-datemath",
"@elastic/elasticsearch": "npm:@elastic/[email protected]",
"@elastic/ems-client": "8.0.0",
Expand Down
2 changes: 2 additions & 0 deletions packages/kbn-test/src/jest/setup/polyfills.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,5 @@ if (!global.URL.hasOwnProperty('createObjectURL')) {
// Will be replaced with a better solution in EUI
// https://github.com/elastic/eui/issues/3713
global._isJest = true;

global.ResizeObserver = require('resize-observer-polyfill');
Copy link
Contributor

@spalger spalger Jan 24, 2022

Choose a reason for hiding this comment

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

It looks like we typically use ResizeObserver via import { ResizeObserver } from 'resize-observer-polyfill' and not via the global. We don't currently do something like this in the browser, and if this polyfill is necessary for @elastic/charts we should probably include the polyfill in packages/kbn-ui-shared-deps-src/src/polyfills.js

Copy link
Member

Choose a reason for hiding this comment

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

Hey @spalger actually the ResizeObserver is supported by all major browsers we support and we don't need anymore the polyfill.
That's why I've removed the polyfill from the @elastic/charts package.

The problem is that the ResizeObserver object is not available in the window object of JSDOM when running the Jest test. Looks like it is not implemented in JSDOM@26.

I don't think we need to include it into packages/kbn-ui-shared-deps-src/src/polyfills.js and I don't think I need to revert our changes in charts to bring back that polyfill. The polyfill should just live in the test environment because is not supported, but we should not include it in our distribution. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I think I've found the issue: in TS the ResizeObserver object is available as a global and it got compiled correctly.
Instead, in JSDOM it is not global but is only a window object.
I've changed the chart compiled code to use window.ResizeObserver instead and the tests are passing. I will push an update into charts soon to fix that

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Actually, this doesn't solve the problem, I can't figure it ATM so I'm reverting back our commit in the elastic-charts and to include back again the polyfill.
This is anyway an issue that needs to be solved: JSDOM doesn't mock/support the ResizeObserver API, Kibana is using 3 years old with no commits resize-observer-polyfill library.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
TooltipProps,
ESFixedIntervalUnit,
ESCalendarIntervalUnit,
PartialTheme,
} from '@elastic/charts';
import type { CustomPaletteState } from '../../../../charts/public';
import { search } from '../../../../data/public';
Expand Down Expand Up @@ -379,62 +380,61 @@ export const HeatmapComponent: FC<HeatmapRenderProps> = memo(
}
};

const config: HeatmapSpec['config'] = {
grid: {
stroke: {
width:
args.gridConfig.strokeWidth ?? chartTheme.axes?.gridLine?.horizontal?.strokeWidth ?? 1,
color:
args.gridConfig.strokeColor ??
chartTheme.axes?.gridLine?.horizontal?.stroke ??
'#D3DAE6',
},
cellHeight: {
max: 'fill',
min: 1,
const themeOverrides: PartialTheme = {
legend: {
labelOptions: {
maxLines: args.legend.shouldTruncate ? args.legend?.maxLines ?? 1 : 0,
},
},
cell: {
maxWidth: 'fill',
maxHeight: 'fill',
label: {
visible: args.gridConfig.isCellLabelVisible ?? false,
minFontSize: 8,
maxFontSize: 18,
useGlobalMinFontSize: true, // override the min if there's a different directive upstream
heatmap: {
grid: {
stroke: {
width:
args.gridConfig.strokeWidth ??
chartTheme.axes?.gridLine?.horizontal?.strokeWidth ??
1,
color:
args.gridConfig.strokeColor ??
chartTheme.axes?.gridLine?.horizontal?.stroke ??
'#D3DAE6',
},
cellHeight: {
max: 'fill',
min: 1,
},
},
border: {
strokeWidth: 0,
cell: {
maxWidth: 'fill',
maxHeight: 'fill',
label: {
visible: args.gridConfig.isCellLabelVisible ?? false,
minFontSize: 8,
maxFontSize: 18,
useGlobalMinFontSize: true, // override the min if there's a different directive upstream
},
border: {
strokeWidth: 0,
},
},
yAxisLabel: {
visible: !!yAxisColumn && args.gridConfig.isYAxisLabelVisible,
// eui color subdued
textColor: chartTheme.axes?.tickLabel?.fill ?? '#6a717d',
padding: yAxisColumn?.name ? 8 : 0,
},
xAxisLabel: {
visible: Boolean(args.gridConfig.isXAxisLabelVisible && xAxisColumn),
// eui color subdued
textColor: chartTheme.axes?.tickLabel?.fill ?? `#6a717d`,
padding: xAxisColumn?.name ? 8 : 0,
},
brushMask: {
fill: isDarkTheme ? 'rgb(30,31,35,80%)' : 'rgb(247,247,247,50%)',
},
brushArea: {
stroke: isDarkTheme ? 'rgb(255, 255, 255)' : 'rgb(105, 112, 125)',
},
},
yAxisLabel: {
visible: !!yAxisColumn && args.gridConfig.isYAxisLabelVisible,
// eui color subdued
textColor: chartTheme.axes?.tickLabel?.fill ?? '#6a717d',
padding: yAxisColumn?.name ? 8 : 0,
name: yAxisColumn?.name ?? '',
...(yAxisColumn
? {
formatter: (v: number | string) =>
`${formatFactory(yAxisColumn.meta.params).convert(v) ?? ''}`,
}
: {}),
},
xAxisLabel: {
visible: Boolean(args.gridConfig.isXAxisLabelVisible && xAxisColumn),
// eui color subdued
textColor: chartTheme.axes?.tickLabel?.fill ?? `#6a717d`,
padding: xAxisColumn?.name ? 8 : 0,
formatter: (v: number | string) => `${xValuesFormatter.convert(v) ?? ''}`,
name: xAxisColumn?.name ?? '',
},
brushMask: {
fill: isDarkTheme ? 'rgb(30,31,35,80%)' : 'rgb(247,247,247,50%)',
},
brushArea: {
stroke: isDarkTheme ? 'rgb(255, 255, 255)' : 'rgb(105, 112, 125)',
},
timeZone,
};

return (
Expand All @@ -454,14 +454,7 @@ export const HeatmapComponent: FC<HeatmapRenderProps> = memo(
legendColorPicker={uiState ? legendColorPicker : undefined}
debugState={window._echDebugStateFlag ?? false}
tooltip={tooltip}
theme={{
...chartTheme,
legend: {
labelOptions: {
maxLines: args.legend.shouldTruncate ? args.legend?.maxLines ?? 1 : 0,
},
},
}}
theme={[themeOverrides, chartTheme]}
xDomain={{
min:
dateHistogramMeta && dateHistogramMeta.timeRange
Expand All @@ -481,15 +474,23 @@ export const HeatmapComponent: FC<HeatmapRenderProps> = memo(
type: 'bands',
bands,
}}
timeZone={timeZone}
data={chartData}
xAccessor={xAccessor}
yAccessor={yAccessor || 'unifiedY'}
valueAccessor={valueAccessor}
valueFormatter={valueFormatter}
xScale={xScale}
ySortPredicate={yAxisColumn ? getSortPredicate(yAxisColumn) : 'dataIndex'}
config={config}
xSortPredicate={xAxisColumn ? getSortPredicate(xAxisColumn) : 'dataIndex'}
xAxisLabelName={xAxisColumn?.name}
yAxisLabelName={yAxisColumn?.name}
xAxisLabelFormatter={(v) => `${xValuesFormatter.convert(v) ?? ''}`}
yAxisLabelFormatter={
yAxisColumn
? (v) => `${formatFactory(yAxisColumn.meta.params).convert(v) ?? ''}`
: undefined
}
/>
</Chart>
</>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
TooltipProps,
TooltipType,
SeriesIdentifier,
PartitionLayout,
} from '@elastic/charts';
import { useEuiTheme } from '@elastic/eui';
import {
Expand Down Expand Up @@ -47,7 +48,7 @@ import {
canFilter,
getFilterClickData,
getFilterEventData,
getConfig,
getPartitionTheme,
getColumns,
getSplitDimensionAccessor,
getColumnByAccessor,
Expand Down Expand Up @@ -251,8 +252,8 @@ const PieComponent = (props: PieComponentProps) => {
return 1;
}, [visData.rows, metricColumn]);

const config = useMemo(
() => getConfig(visParams, chartTheme, dimensions, rescaleFactor),
const themeOverrides = useMemo(
() => getPartitionTheme(visParams, chartTheme, dimensions, rescaleFactor),
[chartTheme, visParams, dimensions, rescaleFactor]
);
const tooltip: TooltipProps = {
Expand Down Expand Up @@ -369,7 +370,9 @@ const PieComponent = (props: PieComponentProps) => {
)}
theme={[
// Chart background should be transparent for the usage at Canvas.
{ ...chartTheme, background: { color: 'transparent' } },
{ background: { color: 'transparent' } },
themeOverrides,
chartTheme,
{
legend: {
labelOptions: {
Expand All @@ -385,6 +388,8 @@ const PieComponent = (props: PieComponentProps) => {
id="pie"
smallMultiples={SMALL_MULTIPLES_ID}
data={visData.rows}
layout={PartitionLayout.sunburst}
specialFirstInnermostSector={false}
valueAccessor={(d: Datum) => getSliceValue(d, metricColumn)}
percentFormatter={(d: number) => percentFormatter.convert(d / 100)}
valueGetter={
Expand All @@ -400,7 +405,6 @@ const PieComponent = (props: PieComponentProps) => {
: metricFieldFormatter.convert(d)
}
layers={layers}
config={config}
topGroove={!visParams.labels.show ? 0 : undefined}
/>
</Chart>
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,7 @@
* Side Public License, v 1.
*/

import {
Datum,
PartitionFillLabel,
PartitionLayer,
ShapeTreeNode,
ArrayEntry,
} from '@elastic/charts';
import { Datum, PartitionLayer, ShapeTreeNode, ArrayEntry } from '@elastic/charts';
import { isEqual } from 'lodash';
import type { FieldFormatsStart } from 'src/plugins/field_formats/public';
import { SeriesLayer, PaletteRegistry, lightenColor } from '../../../../charts/public';
Expand Down Expand Up @@ -137,7 +131,7 @@ export const getLayers = (
formatter: FieldFormatsStart,
syncColors: boolean
): PartitionLayer[] => {
const fillLabel: Partial<PartitionFillLabel> = {
const fillLabel: PartitionLayer['fillLabel'] = {
valueFont: {
fontWeight: 700,
},
Expand Down
Loading