Refactor to remove duplicate onChange based on feedback

WordPress · Dec 14, 2023 · 440cfd1 · 440cfd1
1 parent 62a578e
commit 440cfd1
Show file tree

Hide file tree

Showing 2 changed files with 55 additions and 70 deletions.
diff --git a/packages/components/src/box-control/axial-input-controls.tsx b/packages/components/src/box-control/axial-input-controls.tsx
@@ -75,22 +75,7 @@ export default function AxialInputControls( {
 		}
 	};
 
-	const sliderOnChange = ( side: GroupedSide, next: string ) => {
-		const nextValues = { ...values };
-
-		if ( side === 'vertical' ) {
-			nextValues.top = next;
-			nextValues.bottom = next;
-		}
-
-		if ( side === 'horizontal' ) {
-			nextValues.left = next;
-			nextValues.right = next;
-		}
-		onChange?.( nextValues );
-	};
-
-	const createHandleOnChange = ( side: GroupedSide ) => ( next?: string ) => {
+	const handleOnValueChange = ( side: GroupedSide, next?: string ) => {
 		if ( ! onChange ) {
 			return;
 		}
@@ -161,7 +146,9 @@ export default function AxialInputControls( {
 								parsedQuantity,
 								selectedUnit ?? parsedUnit,
 							].join( '' ) }
-							onChange={ createHandleOnChange( side ) }
+							onChange={ ( newValue ) =>
+								handleOnValueChange( side, newValue )
+							}
 							onUnitChange={ createHandleOnUnitChange( side ) }
 							onFocus={ createHandleOnFocus( side ) }
 							onHoverOn={ createHandleOnHoverOn( side ) }
@@ -175,15 +162,17 @@ export default function AxialInputControls( {
 							aria-labelledby={ inputId }
 							hideLabelFromVision
 							label={ LABELS[ side ] }
-							onChange={ ( newValue ) => {
-								sliderOnChange(
+							onChange={ ( newValue ) =>
+								handleOnValueChange(
 									side,
-									[
-										newValue,
-										selectedUnit ?? parsedUnit,
-									].join( '' )
-								);
-							} }
+									newValue !== undefined
+										? [
+												newValue,
+												selectedUnit ?? parsedUnit,
+										  ].join( '' )
+										: undefined
+								)
+							}
 							min={ 0 }
 							max={
 								CUSTOM_VALUE_SETTINGS[ selectedUnit ?? 'px' ]

diff --git a/packages/components/src/box-control/input-controls.tsx b/packages/components/src/box-control/input-controls.tsx
@@ -14,7 +14,6 @@ import {
 	InputWrapper,
 } from './styles/box-control-styles';
 import type { BoxControlInputControlProps, BoxControlValue } from './types';
-import type { UnitControlProps } from '../unit-control/types';
 
 const noop = () => {};
 
@@ -49,50 +48,43 @@ export default function BoxInputControls( {
 		onChange( nextValues );
 	};
 
-	const sliderOnChange = ( side: keyof BoxControlValue, next: string ) => {
+	const handleOnValueChange = (
+		side: keyof BoxControlValue,
+		next?: string,
+		extra?: { event: React.SyntheticEvent< Element, Event > }
+	) => {
 		const nextValues = { ...values };
-		nextValues[ side ] = next;
-		handleOnChange( nextValues );
-	};
-
-	const createHandleOnChange: (
-		side: keyof BoxControlValue
-	) => UnitControlProps[ 'onChange' ] =
-		( side ) =>
-		( next, { event } ) => {
-			const nextValues = { ...values };
-			const isNumeric =
-				next !== undefined && ! isNaN( parseFloat( next ) );
-			const nextValue = isNumeric ? next : undefined;
-
-			nextValues[ side ] = nextValue;
-
-			/**
-			 * Supports changing pair sides. For example, holding the ALT key
-			 * when changing the TOP will also update BOTTOM.
-			 */
-			// @ts-expect-error - TODO: event.altKey is only present when the change event was
-			// triggered by a keyboard event. Should this feature be implemented differently so
-			// it also works with drag events?
-			if ( event.altKey ) {
-				switch ( side ) {
-					case 'top':
-						nextValues.bottom = nextValue;
-						break;
-					case 'bottom':
-						nextValues.top = nextValue;
-						break;
-					case 'left':
-						nextValues.right = nextValue;
-						break;
-					case 'right':
-						nextValues.left = nextValue;
-						break;
-				}
+		const isNumeric = next !== undefined && ! isNaN( parseFloat( next ) );
+		const nextValue = isNumeric ? next : undefined;
+
+		nextValues[ side ] = nextValue;
+
+		/**
+		 * Supports changing pair sides. For example, holding the ALT key
+		 * when changing the TOP will also update BOTTOM.
+		 */
+		// @ts-expect-error - TODO: event.altKey is only present when the change event was
+		// triggered by a keyboard event. Should this feature be implemented differently so
+		// it also works with drag events?
+		if ( extra?.event.altKey ) {
+			switch ( side ) {
+				case 'top':
+					nextValues.bottom = nextValue;
+					break;
+				case 'bottom':
+					nextValues.top = nextValue;
+					break;
+				case 'left':
+					nextValues.right = nextValue;
+					break;
+				case 'right':
+					nextValues.left = nextValue;
+					break;
 			}
+		}
 
-			handleOnChange( nextValues );
-		};
+		handleOnChange( nextValues );
+	};
 
 	const createHandleOnUnitChange =
 		( side: keyof BoxControlValue ) => ( next?: string ) => {
@@ -132,7 +124,9 @@ export default function BoxInputControls( {
 							value={ [ parsedQuantity, computedUnit ].join(
 								''
 							) }
-							onChange={ createHandleOnChange( side ) }
+							onChange={ ( nextValue, extra ) =>
+								handleOnValueChange( side, nextValue, extra )
+							}
 							onUnitChange={ createHandleOnUnitChange( side ) }
 							onFocus={ createHandleOnFocus( side ) }
 							onHoverOn={ createHandleOnHoverOn( side ) }
@@ -146,9 +140,11 @@ export default function BoxInputControls( {
 							__nextHasNoMarginBottom
 							hideLabelFromVision
 							onChange={ ( newValue ) => {
-								sliderOnChange(
+								handleOnValueChange(
 									side,
-									[ newValue, computedUnit ].join( '' )
+									newValue !== undefined
+										? [ newValue, computedUnit ].join( '' )
+										: undefined
 								);
 							} }
 							min={ 0 }