Merge pull request #65 from adobe/fixMovingAverages

fix: moving averages were broken due to new normalized dimension work

Author: Abigail-Larsen

src/specBuilder/trendline/trendlineUtils.ts

@@ -128,10 +128,10 @@ export const getTrendlineMarks = (markProps: LineSpecProps): GroupMark[] => {
 const getTrendlineLineMark = (markProps: LineSpecProps, trendlineProps: TrendlineSpecProps): LineMark => {
 	const { colorScheme, dimension, scaleType } = markProps;
 	const { displayOnHover, lineType, lineWidth, metric, name, opacity } = trendlineProps;
-	const x =
-		scaleType === 'time'
-			? { scale: 'xTrendline', field: `${dimension}Normalized` }
-			: getXProductionRule(scaleType, dimension);
+
+	const x = trendlineUsesNormalizedDimension(trendlineProps.method, scaleType)
+		? { scale: 'xTrendline', field: `${dimension}Normalized` }
+		: getXProductionRule(scaleType, dimension);
 	const color = trendlineProps.color ? { value: trendlineProps.color } : markProps.color;
 
 	return {
@@ -188,13 +188,8 @@ const getTrendlineHoverMarks = (lineProps: LineSpecProps, highlightRawPoint: boo
 export const addTrendlineData = (data: Data[], markProps: BarSpecProps | LineSpecProps) => {
 	const { dimension } = markProps;
 	data.push(...getTrendlineData(markProps));
-	const trendlines = getTrendlines(markProps.children, markProps.name);
 
-	// only need to add the normalized dimension transform if there is a regression trendline and the dimension is time
-	const hasRegressionTrendline = trendlines.some((trendline) => isRegressionMethod(trendline.method));
-	const hasTimeScale = 'scaleType' in markProps && markProps.scaleType === 'time';
-
-	if (hasRegressionTrendline && hasTimeScale) {
+	if (hasTrendlineWithNormailizedDimension(markProps)) {
 		const tableData = getTableData(data);
 		tableData.transform = addNormalizedDimensionTransform(tableData.transform ?? [], dimension);
 	}
@@ -372,7 +367,9 @@ export const getTrendlineParamFormulaTransforms = (
 	scaleType: ScaleType | undefined
 ): FormulaTransform[] => {
 	let expr = '';
-	const trendlineDimension = scaleType === 'time' ? `${dimension}Normalized` : dimension;
+	const trendlineDimension = trendlineUsesNormalizedDimension(method, scaleType)
+		? `${dimension}Normalized`
+		: dimension;
 	if (isPolynomialMethod(method)) {
 		const order = getPolynomialOrder(method);
 		expr = [
@@ -400,9 +397,9 @@ export const getTrendlineParamFormulaTransforms = (
 };
 
 export const getTrendlineScales = (props: LineSpecProps | BarSpecProps): Scale[] => {
-	const { children, dimension, name } = props;
-	const trendlines = getTrendlines(children, name);
-	if (trendlines.length && 'scaleType' in props && props.scaleType === 'time') {
+	const { dimension } = props;
+
+	if (hasTrendlineWithNormailizedDimension(props)) {
 		return [
 			{
 				name: 'xTrendline',
@@ -421,7 +418,7 @@ export const getTrendlineScales = (props: LineSpecProps | BarSpecProps): Scale[]
 /**
  * determines if the supplied method is a polynomial method
  * @see https://vega.github.io/vega/docs/transforms/regression/
- * @param method regression method
+ * @param method
  * @returns boolean
  */
 const isPolynomialMethod = (method: TrendlineMethod): boolean =>
@@ -430,7 +427,7 @@ const isPolynomialMethod = (method: TrendlineMethod): boolean =>
 /**
  * determines if the supplied method is a regression method
  * @see https://vega.github.io/vega/docs/transforms/regression/
- * @param method regression method
+ * @param method
  * @returns boolean
  */
 const isRegressionMethod = (method: TrendlineMethod): boolean =>
@@ -439,11 +436,34 @@ const isRegressionMethod = (method: TrendlineMethod): boolean =>
 /**
  * determines if the supplied method is a windowing method
  * @see https://vega.github.io/vega/docs/transforms/window/
- * @param method regression method
+ * @param method
  * @returns boolean
  */
 const isWindowMethod = (method: TrendlineMethod): boolean => method.startsWith('movingAverage-');
 
+/**
+ * determines if the supplied method is a regression method that uses the normalized dimension
+ * @see https://vega.github.io/vega/docs/transforms/regression/
+ * @param method
+ * @returns boolean
+ */
+const trendlineUsesNormalizedDimension = (method: TrendlineMethod, scaleType: ScaleType | undefined): boolean =>
+	scaleType === 'time' && isRegressionMethod(method);
+
+/**
+ * determines if any trendlines use the normalized dimension
+ * @param markProps
+ * @returns boolean
+ */
+const hasTrendlineWithNormailizedDimension = (markProps: BarSpecProps | LineSpecProps): boolean => {
+	const trendlines = getTrendlines(markProps.children, markProps.name);
+
+	// only need to add the normalized dimension transform if there is a regression trendline and the dimension is time
+	const hasRegressionTrendline = trendlines.some((trendline) => isRegressionMethod(trendline.method));
+	const hasTimeScale = 'scaleType' in markProps && markProps.scaleType === 'time';
+	return hasRegressionTrendline && hasTimeScale;
+};
+
 /**
  * gets the statistical transforms that will calculate the trendline values
  * @param markProps

src/stories/components/Trendline/Trendline.test.tsx

@@ -23,15 +23,52 @@ describe('Trendline', () => {
 		render(<Trendline />);
 	});
 
-	test('Basic renders properly', async () => {
-		render(<Basic {...Basic.args} />);
-		const chart = await findChart();
-		expect(chart).toBeInTheDocument();
-
-		const trendlines = await findAllMarksByGroupName(chart, 'line0Trendline0');
-		expect(trendlines).toHaveLength(4);
-		expect(trendlines[0]).toHaveAttribute('stroke-dasharray', '7,4');
-		expect(trendlines[0]).toHaveAttribute('stroke-width', '1.5');
+	describe('Basic', () => {
+		test('Basic renders properly', async () => {
+			render(<Basic {...Basic.args} />);
+			const chart = await findChart();
+			expect(chart).toBeInTheDocument();
+
+			const trendlines = await findAllMarksByGroupName(chart, 'line0Trendline0');
+			expect(trendlines).toHaveLength(4);
+			expect(trendlines[0]).toHaveAttribute('stroke-dasharray', '7,4');
+			expect(trendlines[0]).toHaveAttribute('stroke-width', '1.5');
+		});
+
+		/**
+		 * If there is an issue with the trendline, vega defaults to drawing all the x positions at 0.
+		 * This function checks that all the x positions are unique.
+		 * @param path
+		 * @returns
+		 */
+		const allXPathPositionsAreUnique = (path: string): boolean => {
+			const xPositions = path.match(/([+-]?(\d*\.)?\d+),/g)?.map((match) => match.replace(',', '')) ?? [];
+			return new Set(xPositions).size === xPositions.length;
+		};
+
+		test('regression method renders correctly', async () => {
+			render(<Basic {...Basic.args} />);
+			const chart = await findChart();
+			expect(chart).toBeInTheDocument();
+
+			const trendlines = await findAllMarksByGroupName(chart, 'line0Trendline0');
+			expect(trendlines).toHaveLength(4);
+			const trendlinePath = trendlines[0].getAttribute('d');
+			expect(trendlinePath).toBeTruthy();
+			expect(allXPathPositionsAreUnique(trendlinePath ?? '')).toBeTruthy();
+		});
+
+		test('moving average method renders correctly', async () => {
+			render(<Basic {...Basic.args} method="movingAverage-3" />);
+			const chart = await findChart();
+			expect(chart).toBeInTheDocument();
+
+			const trendlines = await findAllMarksByGroupName(chart, 'line0Trendline0');
+			expect(trendlines).toHaveLength(4);
+			const trendlinePath = trendlines[0].getAttribute('d');
+			expect(trendlinePath).toBeTruthy();
+			expect(allXPathPositionsAreUnique(trendlinePath ?? '')).toBeTruthy();
+		});
 	});
 
 	describe('DisplayOnHover', () => {