Skip to content

Commit

Permalink
ref(dashboards): Simplify WidgetCardChart (#83535)
Browse files Browse the repository at this point in the history 
Moving some code around, in preparation to integrate standards widgets.

- Move error handling higher up. Instead of having a bunch of error
panels everywhere, check once at the top of the `render` method
- Move series colorization higher in the component. Moves the code up to
reduce nesting, and makes it easier to return early if needed
- Resolve duplication of JSX. Instead of a conditional, pass `enabled`
to `ReleaseQueries` to prevent unnecessarily loading data
  • Loading branch information
@gggritso
gggritso authored Jan 20, 2025
1 parent ea77199 commit abbeccb
Showing 1 changed file with 50 additions and 108 deletions.
158 changes: 50 additions & 108 deletions static/app/views/dashboards/widgetCard/chart.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,20 +131,8 @@ class WidgetCardChart extends Component<WidgetCardChartProps> {
return !isEqual(currentProps, nextProps);
}

tableResultComponent({
loading,
errorMessage,
tableResults,
}: TableResultProps): React.ReactNode {
tableResultComponent({loading, tableResults}: TableResultProps): React.ReactNode {
const {location, widget, selection} = this.props;
if (errorMessage) {
return (
<StyledErrorPanel>
<IconWarning color="gray500" size="lg" />
</StyledErrorPanel>
);
}

if (typeof tableResults === 'undefined') {
// Align height to other charts.
return <LoadingPlaceholder />;
Expand Down Expand Up @@ -178,19 +166,7 @@ class WidgetCardChart extends Component<WidgetCardChartProps> {
});
}

bigNumberComponent({
loading,
errorMessage,
tableResults,
}: TableResultProps): React.ReactNode {
if (errorMessage) {
return (
<StyledErrorPanel>
<IconWarning color="gray500" size="lg" />
</StyledErrorPanel>
);
}

bigNumberComponent({loading, tableResults}: TableResultProps): React.ReactNode {
if (typeof tableResults === 'undefined' || loading) {
return <BigNumber>{'\u2014'}</BigNumber>;
}
Expand Down Expand Up @@ -291,12 +267,20 @@ class WidgetCardChart extends Component<WidgetCardChartProps> {
showConfidenceWarning,
} = this.props;

if (errorMessage) {
return (
<StyledErrorPanel>
<IconWarning color="gray500" size="lg" />
</StyledErrorPanel>
);
}

if (widget.displayType === 'table') {
return getDynamicText({
value: (
<TransitionChart loading={loading} reloading={loading}>
<LoadingScreen loading={loading} />
{this.tableResultComponent({tableResults, loading, errorMessage})}
{this.tableResultComponent({tableResults, loading})}
</TransitionChart>
),
fixed: <Placeholder height="200px" testId="skeleton-ui" />,
Expand All @@ -308,24 +292,49 @@ class WidgetCardChart extends Component<WidgetCardChartProps> {
<TransitionChart loading={loading} reloading={loading}>
<LoadingScreen loading={loading} />
<BigNumberResizeWrapper>
{this.bigNumberComponent({tableResults, loading, errorMessage})}
{this.bigNumberComponent({tableResults, loading})}
</BigNumberResizeWrapper>
</TransitionChart>
);
}

if (errorMessage) {
return (
<StyledErrorPanel>
<IconWarning color="gray500" size="lg" />
</StyledErrorPanel>
);
}

const {location, selection, onLegendSelectChanged, widgetLegendState} = this.props;
const {start, end, period, utc} = selection.datetime;
const {projects, environments} = selection;

const otherRegex = new RegExp(`(?:.* : ${OTHER}$)|^${OTHER}$`);
const shouldColorOther = timeseriesResults?.some(({seriesName}) =>
seriesName?.match(otherRegex)
);
const colors = timeseriesResults
? (theme.charts
.getColorPalette(timeseriesResults.length - (shouldColorOther ? 3 : 2))
?.slice() as string[])
: [];
// TODO(wmak): Need to change this when updating dashboards to support variable topEvents
if (shouldColorOther) {
colors[colors.length] = theme.chartOther;
}

// Create a list of series based on the order of the fields,
const series = timeseriesResults
? timeseriesResults
.map((values, i: number) => {
let seriesName = '';
if (values.seriesName !== undefined) {
seriesName = isEquation(values.seriesName)
? getEquation(values.seriesName)
: values.seriesName;
}
return {
...values,
seriesName,
fieldName: seriesName,
color: colors[i],
};
})
.filter(Boolean) // NOTE: `timeseriesResults` is a sparse array! We have to filter out the empty slots after the colors are assigned, since the colors are assigned based on sparse array index
: [];

const legend = {
left: 0,
top: 0,
Expand Down Expand Up @@ -461,60 +470,20 @@ class WidgetCardChart extends Component<WidgetCardChartProps> {
},
};

const forwardedRef = this.props.chartGroup ? this.handleRef : undefined;

return (
<ChartZoom period={period} start={start} end={end} utc={utc}>
{zoomRenderProps => {
if (errorMessage) {
return (
<StyledErrorPanel>
<IconWarning color="gray500" size="lg" />
</StyledErrorPanel>
);
}

const otherRegex = new RegExp(`(?:.* : ${OTHER}$)|^${OTHER}$`);
const shouldColorOther = timeseriesResults?.some(({seriesName}) =>
seriesName?.match(otherRegex)
);
const colors = timeseriesResults
? (theme.charts
.getColorPalette(timeseriesResults.length - (shouldColorOther ? 3 : 2))
?.slice() as string[])
: [];
// TODO(wmak): Need to change this when updating dashboards to support variable topEvents
if (shouldColorOther) {
colors[colors.length] = theme.chartOther;
}

// Create a list of series based on the order of the fields,
const series = timeseriesResults
? timeseriesResults
.map((values, i: number) => {
let seriesName = '';
if (values.seriesName !== undefined) {
seriesName = isEquation(values.seriesName)
? getEquation(values.seriesName)
: values.seriesName;
}
return {
...values,
seriesName,
color: colors[i],
};
})
.filter(Boolean) // NOTE: `timeseriesResults` is a sparse array! We have to filter out the empty slots after the colors are assigned, since the colors are assigned based on sparse array index
: [];

const forwardedRef = this.props.chartGroup ? this.handleRef : undefined;

return widgetLegendState.widgetRequiresLegendUnselection(widget) ? (
return (
<ReleaseSeries
end={end}
start={start}
period={period}
environments={environments}
projects={projects}
memoized
enabled={widgetLegendState.widgetRequiresLegendUnselection(widget)}
>
{({releaseSeries}) => {
// make series name into seriesName:widgetId form for individual widget legend control
Expand Down Expand Up @@ -559,33 +528,6 @@ class WidgetCardChart extends Component<WidgetCardChartProps> {
);
}}
</ReleaseSeries>
) : (
<TransitionChart loading={loading} reloading={loading}>
<LoadingScreen loading={loading} />
<ChartWrapper autoHeightResize={shouldResize ?? true} noPadding={noPadding}>
<RenderedChartContainer>
{getDynamicText({
value: this.chartComponent({
...zoomRenderProps,
...chartOptions,
// Override default datazoom behaviour for updating Global Selection Header
...(onZoom ? {onDataZoom: onZoom} : {}),
legend,
series,
onLegendSelectChanged,
forwardedRef,
}),
fixed: <Placeholder height="200px" testId="skeleton-ui" />,
})}
</RenderedChartContainer>
{showConfidenceWarning && confidence && (
<ConfidenceWarning
query={widget.queries[0]?.conditions ?? ''}
confidence={confidence}
/>
)}
</ChartWrapper>
</TransitionChart>
);
}}
</ChartZoom>
Expand Down

0 comments on commit abbeccb

Please sign in to comment.