Skip to content

chore(charts): refactor how skeleton theme is applied #10348

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

Merged
merged 2 commits into from
May 14, 2024
Merged
Show file tree
Hide file tree
Changes from all 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
9 changes: 4 additions & 5 deletions packages/react-charts/src/components/Chart/Chart.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -541,6 +541,7 @@ export const Chart: React.FunctionComponent<ChartProps> = ({
...(name && { name: `${name}-${(legendComponent as any).type.displayName}` }),
orientation: legendOrientation,
theme,
themeColor,
...(legendDirection === 'rtl' && {
dataComponent: legendComponent.props.dataComponent ? (
React.cloneElement(legendComponent.props.dataComponent, { transform: `translate(${legendXOffset})` })
Expand All @@ -552,11 +553,7 @@ export const Chart: React.FunctionComponent<ChartProps> = ({
labelComponent: legendComponent.props.labelComponent ? (
React.cloneElement(legendComponent.props.labelComponent, { direction: 'rtl', dx: legendXOffset - 30 })
) : (
<ChartLabel
direction="rtl"
dx={legendXOffset - 30}
backgroundStyle={theme.skeleton ? theme.skeleton.backgroundStyle : undefined} // override backgroundStyle
/>
<ChartLabel direction="rtl" dx={legendXOffset - 30} />
)
}),
...legendComponent.props
Expand Down Expand Up @@ -603,6 +600,7 @@ export const Chart: React.FunctionComponent<ChartProps> = ({
padding: defaultPadding,
position: legendPosition,
theme,
themeColor,
width,
...(defaultPatternScale && { patternScale: defaultPatternScale })
});
Expand All @@ -621,6 +619,7 @@ export const Chart: React.FunctionComponent<ChartProps> = ({
name: `${name}-${(child as any).type.displayName}-${index}`
}),
theme,
themeColor,
...childProps,
...((child as any).type.displayName === 'ChartPie' && {
data: mergePatternData(childProps.data, defaultPatternScale)
Expand Down
8 changes: 5 additions & 3 deletions packages/react-charts/src/components/ChartAxis/ChartAxis.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import { VictoryAxis, VictoryAxisProps, VictoryAxisTTargetType } from 'victory-a
import { ChartContainer } from '../ChartContainer/ChartContainer';
import { ChartLabel } from '../ChartLabel/ChartLabel';
import { ChartThemeDefinition } from '../ChartTheme/ChartTheme';
import { getTheme } from '../ChartUtils/chart-theme';
import { getComponentTheme, getTheme } from '../ChartUtils/chart-theme';
import { getAxisTheme } from '../ChartUtils/chart-theme-types';

/**
Expand Down Expand Up @@ -451,6 +451,8 @@ export const ChartAxis: React.FunctionComponent<ChartAxisProps> = ({
theme = getTheme(themeColor),
...rest
}: ChartAxisProps) => {
const componentTheme = getComponentTheme(themeColor);

// Clone so users can override container props
const container = React.cloneElement(containerComponent, {
theme,
Expand All @@ -463,7 +465,7 @@ export const ChartAxis: React.FunctionComponent<ChartAxisProps> = ({
id: () => `${name}-${(axisLabelComponent as any).type.displayName}`
}),
...axisLabelComponent.props,
...(theme.skeleton && theme.skeleton) // override backgroundStyle
...(componentTheme?.label && componentTheme.label) // override backgroundStyle
});

const getTickLabelComponent = () =>
Expand All @@ -472,7 +474,7 @@ export const ChartAxis: React.FunctionComponent<ChartAxisProps> = ({
id: (props: any) => `${name}-${(tickLabelComponent as any).type.displayName}-${props.index}`
}),
...tickLabelComponent.props,
...(theme.skeleton && theme.skeleton) // override backgroundStyle
...(componentTheme?.label && componentTheme.label) // override backgroundStyle
});

// Note: containerComponent is required for theme
Expand Down
10 changes: 2 additions & 8 deletions packages/react-charts/src/components/ChartBullet/ChartBullet.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -594,7 +594,6 @@ export const ChartBullet: React.FunctionComponent<ChartBulletProps> = ({
standalone: false,
subTitle: groupSubTitle,
title: groupTitle,
theme,
themeColor,
width,
...groupTitleComponent.props
Expand Down Expand Up @@ -719,15 +718,10 @@ export const ChartBullet: React.FunctionComponent<ChartBulletProps> = ({
labelComponent: legendComponent.props.labelComponent ? (
React.cloneElement(legendComponent.props.labelComponent, {
direction: 'rtl',
dx: legendXOffset - 30,
...(theme.skeleton && theme.skeleton) // override backgroundStyle
dx: legendXOffset - 30
})
) : (
<ChartLabel
direction="rtl"
dx={legendXOffset - 30}
{...(theme.skeleton && theme.skeleton)} // override backgroundStyle
/>
<ChartLabel direction="rtl" dx={legendXOffset - 30} />
)
}),
...legendComponent.props
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,7 @@ export const ChartBulletComparativeErrorMeasure: React.FunctionComponent<ChartBu
padding,
standalone: false,
theme,
themeColor,
width,
y,
...measureComponent.props
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { ChartBulletStyles } from '../ChartTheme/ChartStyles';
import { getLabelTextSize, getBulletLabelX, getBulletLabelY } from '../ChartUtils/chart-label';
import { getPaddingForSide } from '../ChartUtils/chart-padding';
import { getBulletGroupTitleTheme } from '../ChartUtils/chart-theme-types';
import { getComponentTheme } from '../ChartUtils/chart-theme';

/**
* ChartBulletGroupTitle renders a group title.
Expand Down Expand Up @@ -165,6 +166,7 @@ export const ChartBulletGroupTitle: React.FunctionComponent<ChartBulletGroupTitl

// Returns title
const getTitle = () => {
const componentTheme = getComponentTheme(themeColor);
const titleProps = titleComponent ? titleComponent.props : {};
const showBoth = title && subTitle;
return React.cloneElement(titleComponent, {
Expand All @@ -184,7 +186,7 @@ export const ChartBulletGroupTitle: React.FunctionComponent<ChartBulletGroupTitl
labelPosition: 'top'
}),
...titleProps,
...(theme.skeleton && theme.skeleton) // override backgroundStyle
...(componentTheme?.label && componentTheme.label) // override backgroundStyle
});
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,7 @@ export const ChartBulletPrimaryDotMeasure: React.FunctionComponent<ChartBulletPr
}
},
theme,
themeColor,
width,
...measureComponent.props
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,7 @@ export const ChartBulletPrimarySegmentedMeasure: React.FunctionComponent<ChartBu
}
},
theme,
themeColor,
width,
...measureComponent.props
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,7 @@ export const ChartBulletQualitativeRange: React.FunctionComponent<ChartBulletQua
}
},
theme,
themeColor,
width,
...measureComponent.props
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { ChartThemeDefinition } from '../ChartTheme/ChartTheme';
import { getBulletLabelX, getBulletLabelY } from '../ChartUtils/chart-label';
import { getPaddingForSide } from '../ChartUtils/chart-padding';
import { getBulletTheme } from '../ChartUtils/chart-theme-types';
import { getComponentTheme } from '../ChartUtils/chart-theme';

/**
* ChartBulletTitle renders the bullet chart title.
Expand Down Expand Up @@ -159,6 +160,7 @@ export const ChartBulletTitle: React.FunctionComponent<ChartBulletTitleProps> =

// Returns title
const getTitle = () => {
const componentTheme = getComponentTheme(themeColor);
const showBoth = title && subTitle;

let labelPosition: 'bottom' | 'left' | 'top-left' = horizontal ? 'left' : 'bottom';
Expand Down Expand Up @@ -216,7 +218,7 @@ export const ChartBulletTitle: React.FunctionComponent<ChartBulletTitleProps> =
labelPosition
}),
...titleComponent.props,
...(theme.skeleton && theme.skeleton) // override backgroundStyle
...(componentTheme?.label && componentTheme.label) // override backgroundStyle
});
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {
} from 'victory-cursor-container';
import { ChartLabel } from '../ChartLabel/ChartLabel';
import { ChartThemeDefinition } from '../ChartTheme/ChartTheme';
import { getTheme } from '../ChartUtils/chart-theme';
import { getComponentTheme, getTheme } from '../ChartUtils/chart-theme';
import { getClassName } from '../ChartUtils/chart-helpers';

/**
Expand Down Expand Up @@ -210,11 +210,12 @@ export const ChartCursorContainer: React.FunctionComponent<ChartCursorContainerP
cursorLabelComponent = <ChartLabel />, // Note that Victory provides its own label component here
...rest
}: ChartCursorContainerProps) => {
const componentTheme = getComponentTheme(themeColor);
const chartClassName = getClassName({ className });
const chartCursorLabelComponent = React.cloneElement(cursorLabelComponent, {
theme,
...cursorLabelComponent.props,
...(theme.skeleton && theme.skeleton) // override backgroundStyle
...(componentTheme?.label && componentTheme.label) // override backgroundStyle
});

// Clone so users can override cursor container props
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import { ChartCommonStyles } from '../ChartTheme/ChartStyles';
import { ChartThemeDefinition } from '../ChartTheme/ChartTheme';
import { getPaddingForSide } from '../ChartUtils/chart-padding';
import { getPieLabelX, getPieLabelY } from '../ChartUtils/chart-label';
import { getComponentTheme } from '../ChartUtils/chart-theme';

interface ChartDonutSubTitleInterface {
dy?: number;
Expand Down Expand Up @@ -596,6 +597,8 @@ export const ChartDonut: React.FunctionComponent<ChartDonutProps> = ({
width = theme.pie.width,
...rest
}: ChartDonutProps) => {
const componentTheme = getComponentTheme(themeColor);

const defaultPadding = {
bottom: getPaddingForSide('bottom', padding, theme.pie.padding),
left: getPaddingForSide('left', padding, theme.pie.padding),
Expand Down Expand Up @@ -660,7 +663,7 @@ export const ChartDonut: React.FunctionComponent<ChartDonutProps> = ({
width
}),
...subTitleProps,
...(theme.skeleton && theme.skeleton) // override backgroundStyle
...(componentTheme?.label && componentTheme.label) // override backgroundStyle
});
};

Expand Down Expand Up @@ -694,7 +697,7 @@ export const ChartDonut: React.FunctionComponent<ChartDonutProps> = ({
width
}),
...titleProps,
...(theme.skeleton && theme.skeleton) // override backgroundStyle
...(componentTheme?.label && componentTheme.label) // override backgroundStyle
});
};

Expand All @@ -712,6 +715,7 @@ export const ChartDonut: React.FunctionComponent<ChartDonutProps> = ({
radius={chartRadius > 0 ? chartRadius : 0}
standalone={false}
theme={theme}
themeColor={themeColor}
width={width}
{...rest}
/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -524,6 +524,7 @@ export const ChartDonutThreshold: React.FunctionComponent<ChartDonutThresholdPro
standalone: false,
subTitlePosition: childProps.subTitlePosition || subTitlePosition,
theme: dynamicTheme,
themeColor,
width,
...childProps
});
Expand All @@ -546,6 +547,7 @@ export const ChartDonutThreshold: React.FunctionComponent<ChartDonutThresholdPro
padding={defaultPadding}
standalone={false}
theme={theme}
themeColor={themeColor}
width={width}
{...rest}
/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -688,6 +688,7 @@ export const ChartDonutUtilization: React.FunctionComponent<ChartDonutUtilizatio
padding={padding}
standalone={false}
theme={getThresholdTheme()}
themeColor={themeColor}
width={width}
{...rest}
/>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -499,7 +499,8 @@ export const ChartGroup: React.FunctionComponent<ChartGroupProps> = ({
<VictoryGroup colorScale={colorScale} containerComponent={container} theme={theme} {...rest}>
{renderChildrenWithPatterns({
children,
patternScale: defaultPatternScale
patternScale: defaultPatternScale,
themeColor
})}
{isPatternDefs && getPatternDefs({ patternId, colorScale: defaultColorScale })}
</VictoryGroup>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import { ChartContainer } from '../ChartContainer/ChartContainer';
import { ChartLabel } from '../ChartLabel/ChartLabel';
import { ChartPoint } from '../ChartPoint/ChartPoint';
import { ChartThemeDefinition } from '../ChartTheme/ChartTheme';
import { getTheme } from '../ChartUtils/chart-theme';
import { getComponentTheme, getTheme } from '../ChartUtils/chart-theme';

/**
* ChartLegend renders a chart legend component.
Expand Down Expand Up @@ -317,6 +317,8 @@ export const ChartLegend: React.FunctionComponent<ChartLegendProps> = ({
theme = getTheme(themeColor),
...rest
}: ChartLegendProps) => {
const componentTheme = getComponentTheme(themeColor);

// Merge pattern IDs with `style.data.fill` property
const getDefaultStyle = () => {
if (!patternScale) {
Expand Down Expand Up @@ -351,15 +353,15 @@ export const ChartLegend: React.FunctionComponent<ChartLegendProps> = ({
React.cloneElement(labelComponent, {
...(name && { id: (props: any) => `${name}-${(labelComponent as any).type.displayName}-${props.index}` }),
...labelComponent.props,
...(theme.skeleton && theme.skeleton) // override backgroundStyle
...(componentTheme?.label && componentTheme.label) // override backgroundStyle
});

const getTitleComponent = () =>
React.cloneElement(titleComponent, {
// Victory doesn't appear to call the id function here, but it's valid for label components
...(name && { id: () => `${name}-${(titleComponent as any).type.displayName}` }),
...titleComponent.props,
...(theme.skeleton && theme.skeleton) // override backgroundStyle
...(componentTheme?.label && componentTheme.label) // override backgroundStyle
});

// Note: containerComponent is required for theme
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -401,6 +401,7 @@ export const ChartLegendTooltip: React.FunctionComponent<ChartLegendTooltipProps
}),
text,
theme,
themeColor,
width,
...rest
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,7 @@ export const ChartLegendTooltipContent: React.FunctionComponent<ChartLegendToolt
patternScale,
standalone: false,
theme,
themeColor,
x: getX() + legendOffsetX + Helpers.evaluateProp(dx, undefined),
y: getY() + legendOffsetY + Helpers.evaluateProp(dy, undefined),
...legendProps
Expand Down
1 change: 1 addition & 0 deletions packages/react-charts/src/components/ChartPie/ChartPie.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -593,6 +593,7 @@ export const ChartPie: React.FunctionComponent<ChartPieProps> = ({
key: 'pf-chart-pie-legend',
orientation: legendOrientation,
theme,
themeColor,
...(legendDirection === 'rtl' && {
dataComponent: legendComponent.props.dataComponent ? (
React.cloneElement(legendComponent.props.dataComponent, { transform: `translate(${legendXOffset})` })
Expand Down
41 changes: 38 additions & 3 deletions packages/react-charts/src/components/ChartTheme/ChartTheme.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,26 @@
import { VictoryThemeDefinition } from 'victory-core';

// Note: Victory incorrectly typed ThemeBaseProps.padding as number instead of PaddingProps
export interface ChartThemeDefinitionInterface extends VictoryThemeDefinition {
skeleton?: {
/**
* Chart component theme definition
* @private
* @beta
*/
export interface ChartComponentThemeDefinitionInterface {
axis?: VictoryThemeDefinition;
Copy link
Contributor

Choose a reason for hiding this comment

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

It's unexpected to me that type of each of these props is now VictoryThemeDefinition.

I guess it's because the types spread all the same props in this API doc.

So i guess that's not a comment that requires any action. just me documenting what I learned for maybe future me 😇

Copy link
Member Author

@dlabrecq dlabrecq May 8, 2024

Choose a reason for hiding this comment

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

This particular object may contain any of the theme props defined by VictoryThemeDefinition. For example, our custom donut utilization component overrides props for both Victory pie and legend. However, a custom component could override even more Victory theme props here (e.g., tooltip), if necessary.

bullet?: VictoryThemeDefinition;
bulletComparativeErrorMeasure?: VictoryThemeDefinition;
bulletComparativeMeasure?: VictoryThemeDefinition;
bulletComparativeWarningMeasure: VictoryThemeDefinition;
bulletGroupTitle?: VictoryThemeDefinition;
bulletPrimaryDotMeasure?: VictoryThemeDefinition;
bulletPrimaryNegativeMeasure?: VictoryThemeDefinition;
bulletPrimarySegmentedMeasure?: VictoryThemeDefinition;
bulletQualitativeRange?: VictoryThemeDefinition;
donut?: VictoryThemeDefinition;
donutThresholdDynamic?: VictoryThemeDefinition;
donutThresholdStatic?: VictoryThemeDefinition;
donutUtilization?: VictoryThemeDefinition;
label?: {
backgroundStyle?: {
fill?: string;
};
Expand All @@ -11,10 +29,27 @@ export interface ChartThemeDefinitionInterface extends VictoryThemeDefinition {
stroke?: string;
};
};
threshold?: VictoryThemeDefinition;
}

/**
* Chart theme definition
*
* Note: Victory incorrectly typed ThemeBaseProps.padding as number instead of PaddingProps
*
* @public
*/
export interface ChartThemeDefinitionInterface extends VictoryThemeDefinition {}

/**
* Chart theme definition
* @public
*/
export type ChartThemeDefinition = ChartThemeDefinitionInterface;

/**
* Chart component theme definition
* @private
* @beta
*/
export type ChartComponentThemeDefinition = ChartComponentThemeDefinitionInterface;
Loading
Loading