fix(APP-3440): Fix NumberInput component to correctly update value on…

… buttons click (#262)
aragon · Aug 6, 2024 · 2b0d109 · 2b0d109
1 parent ed2b841
commit 2b0d109
Show file tree

Hide file tree

Showing 9 changed files with 86 additions and 95 deletions.
diff --git a/.github/dependabot.yml b/.github/dependabot.yml
@@ -4,6 +4,10 @@ updates:
     directory: "/"
     schedule:
       interval: "weekly"
+    ignore:
+      # From version 7.6.x, react-imask doesn't correctly trigger the `onAccept` callback when changing the value
+      # programmatically (see https://github.com/uNmAnNeR/imaskjs/pull/1045)
+      - dependency-name: "react-imask"
     groups:
       minor-and-patch:
         update-types:

diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -15,6 +15,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 -   Fix ENS name truncation on `<Wallet />` module component
 -   Update `<Wallet />` module component to only resolve user ENS name when name property is not set
 -   Fix expand behaviour of `TextAreaRichText` core component when used inside a dialog and hide the input label
+-   Fix `NumberInput` component to correctly update values on plus / minus buttons click
 
 ### Added
 

diff --git a/package.json b/package.json
@@ -68,7 +68,7 @@
     "framer-motion": "^11.3.0",
     "luxon": "^3.5.0",
     "react-dropzone": "^14.2.0",
-    "react-imask": "^7.6.0",
+    "react-imask": "7.5.0",
     "sanitize-html": "^2.13.0",
     "tiptap-markdown": "^0.8.10"
   },

diff --git a/src/core/components/forms/hooks/useNumberMask.ts b/src/core/components/forms/hooks/useNumberMask.ts
@@ -33,14 +33,16 @@ const getNumberSeparators = () => {
 };
 
 // The imask.js library requires us to set a "scale" property as max decimal places otherwise it defaults to 0.
-const maxDecimalPlaces = 30;
+const defaultScale = 30;
 
 export const useNumberMask = (props: IUseNumberMaskProps): IUseNumberMaskResult => {
     const { suffix, prefix, min, max, onChange, value } = props;
 
     const { thousandsSeparator, radix } = getNumberSeparators();
 
     const numberMask = `${prefix ?? ''} num ${suffix ?? ''}`.trim();
+    const maskMax = max != null ? Number(max) : undefined;
+    const maskMin = min != null ? Number(min) : undefined;
 
     const handleMaskAccept = (_value: string, mask: InputMask<FactoryOpts>) => {
         // Update the lazy option to display the suffix when the user is deleting the last digits of the input
@@ -54,14 +56,7 @@ export const useNumberMask = (props: IUseNumberMaskProps): IUseNumberMaskResult
             mask: numberMask,
             eager: true, // Displays eventual suffix on user input
             blocks: {
-                num: {
-                    mask: Number,
-                    radix,
-                    thousandsSeparator,
-                    scale: maxDecimalPlaces,
-                    max: max != null ? Number(max) : undefined,
-                    min: min != null ? Number(min) : undefined,
-                },
+                num: { mask: Number, radix, thousandsSeparator, scale: defaultScale, max: maskMax, min: maskMin },
             },
         },
         { onAccept: handleMaskAccept },

diff --git a/src/core/components/forms/inputNumber/inputNumber.test.tsx b/src/core/components/forms/inputNumber/inputNumber.test.tsx
@@ -5,6 +5,16 @@ import * as InputHooks from '../hooks';
 import { InputNumber, type IInputNumberProps } from './inputNumber';
 
 describe('<InputNumber /> component', () => {
+    const useNumberMaskMock = jest.spyOn(InputHooks, 'useNumberMask');
+
+    beforeEach(() => {
+        useNumberMaskMock.mockReturnValue({} as unknown as InputHooks.IUseNumberMaskResult);
+    });
+
+    afterEach(() => {
+        useNumberMaskMock.mockReset();
+    });
+
     const createTestComponent = (props?: Partial<IInputNumberProps>) => {
         const completeProps: IInputNumberProps = {
             ...props,
@@ -13,6 +23,34 @@ describe('<InputNumber /> component', () => {
         return <InputNumber {...completeProps} />;
     };
 
+    const testChangeValueLogic = async (values?: {
+        props?: Partial<IInputNumberProps>;
+        expectedValue?: string;
+        type: 'increment' | 'decrement';
+    }) => {
+        const { props, expectedValue, type } = values ?? {};
+        const user = userEvent.setup();
+        const setUnmaskedValue = jest.fn();
+
+        const hookResult = {
+            setUnmaskedValue,
+            unmaskedValue: props?.value,
+        } as unknown as InputHooks.IUseNumberMaskResult;
+
+        useNumberMaskMock.mockReturnValue(hookResult);
+        render(createTestComponent({ ...props }));
+
+        const [decrementButton, incrementButton] = screen.getAllByRole('button');
+
+        if (type === 'increment') {
+            await user.click(incrementButton);
+        } else {
+            await user.click(decrementButton);
+        }
+
+        expect(setUnmaskedValue).toHaveBeenCalledWith(expectedValue);
+    };
+
     it('renders an input with increment and decrement buttons', () => {
         render(createTestComponent());
 
@@ -42,109 +80,61 @@ describe('<InputNumber /> component', () => {
     });
 
     describe('increment button', () => {
-        const useNumberMaskMock = jest.spyOn(InputHooks, 'useNumberMask');
-
-        afterEach(() => {
-            useNumberMaskMock.mockReset();
-        });
-
-        const testIncrementLogic = async ({
-            expectedValue,
-            ...props
-        }: Partial<IInputNumberProps> & { expectedValue: string }) => {
-            const user = userEvent.setup();
-            const setValue = jest.fn();
-            const hookResult = {
-                setValue,
-                value: props.value,
-                unmaskedValue: props.value,
-            } as unknown as InputHooks.IUseNumberMaskResult;
-            useNumberMaskMock.mockReturnValue(hookResult);
-
-            render(createTestComponent({ ...props }));
-
-            const [, incrementButton] = screen.getAllByRole<HTMLButtonElement>('button');
-            await user.click(incrementButton);
-
-            expect(setValue).toHaveBeenCalledWith(expectedValue);
-        };
-
         it('should increment by one (1) with default parameters', async () => {
-            await testIncrementLogic({ expectedValue: '1' });
+            await testChangeValueLogic({ type: 'increment', expectedValue: '1' });
         });
 
         it('should return the maximum when the newly generated value exceeds the maximum', async () => {
             const max = 5;
             const step = 2;
             const value = '4';
-            await testIncrementLogic({ max, step, value, expectedValue: max.toString() });
+            const props = { max, step, value };
+            await testChangeValueLogic({ type: 'increment', props, expectedValue: max.toString() });
         });
 
         it('should increment by floating point value when the step is a float', async () => {
             const value = '1';
             const step = 0.5;
-            await testIncrementLogic({ step, value, expectedValue: (Number(value) + step).toString() });
+            const props = { step, value };
+            await testChangeValueLogic({ type: 'increment', props, expectedValue: (Number(value) + step).toString() });
         });
 
         it('should round down to the nearest multiple of the step before incrementing by the step value', async () => {
             const value = '1';
             const step = 0.3;
-            await testIncrementLogic({ step, value, expectedValue: '1.2' });
+            const props = { value, step };
+            await testChangeValueLogic({ type: 'increment', props, expectedValue: '1.2' });
         });
 
         it('should increment to the minimum when no value is provided', async () => {
             const step = 6;
             const min = 5;
             const max = 10;
-            await testIncrementLogic({ step, min, max, expectedValue: min.toString() });
+            const props = { step, min, max };
+            await testChangeValueLogic({ type: 'increment', props, expectedValue: min.toString() });
         });
     });
 
     describe('decrement button', () => {
-        const useNumberMaskMock = jest.spyOn(InputHooks, 'useNumberMask');
-
-        afterEach(() => {
-            useNumberMaskMock.mockReset();
-        });
-
-        const testDecrementLogic = async ({
-            expectedValue,
-            ...props
-        }: Partial<IInputNumberProps> & { expectedValue: string }) => {
-            const user = userEvent.setup();
-            const setValue = jest.fn();
-            const hookResult = {
-                setValue,
-                value: props.value,
-                unmaskedValue: props.value,
-            } as unknown as InputHooks.IUseNumberMaskResult;
-            useNumberMaskMock.mockReturnValue(hookResult);
-
-            render(createTestComponent({ ...props }));
-
-            const [decrementButton] = screen.getAllByRole<HTMLButtonElement>('button');
-            await user.click(decrementButton);
-
-            expect(setValue).toHaveBeenCalledWith(expectedValue);
-        };
-
         it('should decrement by step', async () => {
             const value = '10';
             const step = 2;
-            const expectedValue = (10 - 2).toString();
-            await testDecrementLogic({ value, step, expectedValue });
+            const props = { value, step };
+            await testChangeValueLogic({ type: 'decrement', props, expectedValue: (10 - 2).toString() });
         });
 
         it('should decrement to the minimum when no value provided', async () => {
             const step = 2;
             const min = 1;
-            await testDecrementLogic({ step, min, expectedValue: min.toString() });
+            const props = { step, min };
+            await testChangeValueLogic({ type: 'decrement', props, expectedValue: min.toString() });
         });
 
         it('should decrement to the closest multiple of the step smaller than the value', async () => {
             const value = '10';
             const step = 3;
-            await testDecrementLogic({ value, step, expectedValue: '9' });
+            const props = { value, step };
+            await testChangeValueLogic({ type: 'decrement', props, expectedValue: '9' });
         });
     });
 });
diff --git a/src/core/components/forms/inputNumber/inputNumber.tsx b/src/core/components/forms/inputNumber/inputNumber.tsx
@@ -24,7 +24,8 @@ export interface IInputNumberProps
      */
     prefix?: string;
     /**
-     * Specifies the granularity of the intervals for the input value
+     * Specifies the granularity of the intervals for the input value.
+     * @default 1
      */
     step?: number;
     /**
@@ -41,14 +42,14 @@ export const InputNumber = forwardRef<HTMLInputElement, IInputNumberProps>((prop
     const {
         max = Number.MAX_SAFE_INTEGER,
         min = Number.MIN_SAFE_INTEGER,
-        step: inputStep = 1,
+        step = 1,
         prefix,
         suffix,
         onChange,
         ...otherProps
     } = props;
 
-    const step = inputStep <= 0 ? 1 : inputStep;
+    const processedStep = step <= 0 ? 1 : step;
     const { containerProps, inputProps } = useInputProps(otherProps);
 
     const { className: containerClassName, disabled, ...otherContainerProps } = containerProps;
@@ -57,7 +58,7 @@ export const InputNumber = forwardRef<HTMLInputElement, IInputNumberProps>((prop
     const {
         ref: maskRef,
         unmaskedValue,
-        setValue,
+        setUnmaskedValue,
     } = useNumberMask({
         min,
         max,
@@ -82,31 +83,31 @@ export const InputNumber = forwardRef<HTMLInputElement, IInputNumberProps>((prop
 
         // increment directly to the minimum if value is less than the minimum
         if (parsedValue < min) {
-            setValue(min.toString());
+            setUnmaskedValue(min.toString());
             return;
         }
 
         // ensure value is multiple of step
         const newValue = (Math.floor(parsedValue / step) + 1) * step;
 
         // ensure the new value is than the max
-        setValue(Math.min(max, newValue).toString());
+        setUnmaskedValue(Math.min(max, newValue).toString());
     };
 
     const handleDecrement = () => {
         const parsedValue = Number(unmaskedValue ?? 0);
 
         // decrement directly to the maximum if value is greater than the maximum
         if (parsedValue > max) {
-            setValue(max.toString());
+            setUnmaskedValue(max.toString());
             return;
         }
 
         // ensure value is multiple of step
         const newValue = (Math.ceil(parsedValue / step) - 1) * step;
 
         // ensure the new value is than the max
-        setValue(Math.max(min, newValue).toString());
+        setUnmaskedValue(Math.max(min, newValue).toString());
     };
 
     return (
@@ -122,7 +123,7 @@ export const InputNumber = forwardRef<HTMLInputElement, IInputNumberProps>((prop
             )}
             <input
                 ref={mergeRefs([maskRef, ref])}
-                step={step}
+                step={processedStep}
                 max={max}
                 min={min}
                 inputMode="numeric"

diff --git a/src/core/components/forms/inputNumberMax/inputNumberMax.test.tsx b/src/core/components/forms/inputNumberMax/inputNumberMax.test.tsx
@@ -10,7 +10,7 @@ describe('<InputNumberMax /> component', () => {
     beforeEach(() => {
         const numberMaskResult = {
             ref: createRef(),
-            setValue: jest.fn(),
+            setUnmaskedValue: jest.fn(),
         } as unknown as InputHooks.IUseNumberMaskResult;
         useNumberMaskMock.mockReturnValue(numberMaskResult);
     });
@@ -37,12 +37,12 @@ describe('<InputNumberMax /> component', () => {
     it('updates the mask value with the max property on max button click', async () => {
         const user = userEvent.setup();
         const max = 1_000_000;
-        const setValue = jest.fn();
-        const hookResult = { setValue } as unknown as InputHooks.IUseNumberMaskResult;
+        const setUnmaskedValue = jest.fn();
+        const hookResult = { setUnmaskedValue } as unknown as InputHooks.IUseNumberMaskResult;
         useNumberMaskMock.mockReturnValue(hookResult);
         render(createTestComponent({ max }));
         await user.click(screen.getByRole('button'));
-        expect(setValue).toHaveBeenCalledWith(max.toString());
+        expect(setUnmaskedValue).toHaveBeenCalledWith(max.toString());
     });
 
     it('does not render the max button when input is disabled', () => {

diff --git a/src/core/components/forms/inputNumberMax/inputNumberMax.tsx b/src/core/components/forms/inputNumberMax/inputNumberMax.tsx
@@ -22,11 +22,11 @@ export const InputNumberMax: React.FC<IInputNumberMaxProps> = (props) => {
     const { variant, ...otherContainerProps } = containerProps;
     const { className: inputClassName, value, min, disabled, ...otherInputProps } = inputProps;
 
-    const { ref, setValue } = useNumberMask({ min, max, value, onChange });
+    const { ref, setUnmaskedValue } = useNumberMask({ min, max, value, onChange });
 
     const { copy } = useOdsCoreContext();
 
-    const handleMaxClick = () => setValue(max.toString());
+    const handleMaxClick = () => setUnmaskedValue(max.toString());
 
     return (
         <InputContainer variant={variant} {...otherContainerProps}>

diff --git a/yarn.lock b/yarn.lock
@@ -119,7 +119,7 @@ __metadata:
     react: "npm:^18.3.1"
     react-dom: "npm:^18.3.1"
     react-dropzone: "npm:^14.2.0"
-    react-imask: "npm:^7.6.0"
+    react-imask: "npm:7.5.0"
     rollup: "npm:^4.20.0"
     rollup-plugin-peer-deps-external: "npm:^2.2.4"
     rollup-plugin-postcss: "npm:^4.0.2"
@@ -11077,7 +11077,7 @@ __metadata:
   languageName: node
   linkType: hard
 
-"imask@npm:^7.6.1":
+"imask@npm:^7.5.0":
   version: 7.6.1
   resolution: "imask@npm:7.6.1"
   dependencies:
@@ -15513,15 +15513,15 @@ __metadata:
   languageName: node
   linkType: hard
 
-"react-imask@npm:^7.6.0":
-  version: 7.6.1
-  resolution: "react-imask@npm:7.6.1"
+"react-imask@npm:7.5.0":
+  version: 7.5.0
+  resolution: "react-imask@npm:7.5.0"
   dependencies:
-    imask: "npm:^7.6.1"
+    imask: "npm:^7.5.0"
     prop-types: "npm:^15.8.1"
   peerDependencies:
     react: ">=0.14.0"
-  checksum: 10c0/48b8c234fb77e4d8b8446712695c63f2b0351a896b17623510a648408dd1b7ab506496124453a821528781b05fe2f5b04514f902e260f8b7f4486c972061da88
+  checksum: 10c0/755969ce03067608bc543731397b71586d2820094f94cf73e4bcce6c19214ccca7574f160456eee66537f1f64349a542c041df0ecd4b1afeb300f5a270fc31e2
   languageName: node
   linkType: hard