Slider: Add support for decimal values (#113473)

* UI: Fix Slider component to handle decimal inputs correctly * format code and run eslint fix * fix Slider to have: - "single" source of truth for state - state synchronization for controlled values - clamp values to step - disallow decimal values in input when min+step are integers - tests for the new functionality - design decision included in docs - behavior notes in docs * allow non-numeric characters all the time always parse decimal numbers, stripping non-numerics integer coercion is implicitly handled in clamping --------- Co-authored-by: Harshada Gawas <harshadagawas95@gmail.com>
2025-11-10 14:41:39 -05:00
parent dd0a2d4cff
commit b6bd31a4aa
3 changed files with 260 additions and 53 deletions
@@ -9,4 +9,16 @@ The `Slider` component is an input element where users can manipulate one value

 `Slider` can be implemented in horizontal or vertical orientation. You can set the default starting value(s) for the slider with the `value` prop.

+## Behavior
+
+The slider input itself will only allow values from `min` to `max` in steps of `step`.
+
+The text input behavior depends on whether the step value or min value are integers or decimals/fractional. When the min and step are integers, the text input will only allow numeric characters and leading `-`. When the min or step are decimal/fractional, the input will allow a leading `-`, leading `.`, and valid combinations of those, as well as `.` within the number.
+
+`onChange` will only be called with valid values, clamped to the min, max, and step. If `min - max` isn't evenly divisible by `step`, behavior may be unexpected near the maximum.
+
 <ArgTypes of={SliderProps} />
+
+## Design decisions
+
+The slider state deals with two sources of truth: the value that's passed in, and the text input value. The text input value needs to be synchronized with external value changes, but it can also have its own temporarily invalid values.
@@ -4,6 +4,8 @@ import userEvent from '@testing-library/user-event';
 import { Slider } from './Slider';
 import { SliderProps } from './types';

+import '@testing-library/jest-dom';
+
 const sliderProps: SliderProps = {
  min: 10,
  max: 20,
@@ -17,6 +19,37 @@ describe('Slider', () => {
    user = userEvent.setup();
  });

+  it('respects min/max bounds after decimal input blur', async () => {
+    render(<Slider {...sliderProps} min={0} max={10} value={5} />);
+
+    const sliderInput = screen.getByRole('textbox');
+
+    // Above max
+    await user.clear(sliderInput);
+    await user.type(sliderInput, '15.2');
+    await user.click(document.body);
+    expect(sliderInput).toHaveValue('10'); // max enforced
+
+    // Below min
+    await user.clear(sliderInput);
+    await user.type(sliderInput, '-2.7');
+    await user.click(document.body);
+    expect(sliderInput).toHaveValue('0'); // min enforced
+  });
+
+  it('updates slider value correctly when decimal input is typed', async () => {
+    render(<Slider {...sliderProps} min={0} max={10} step={0.2} value={5} />);
+
+    const slider = screen.getByRole('slider');
+    const sliderInput = screen.getByRole('textbox');
+
+    await user.clear(sliderInput);
+    await user.type(sliderInput, '7.3');
+    await user.click(document.body);
+
+    expect(slider).toHaveAttribute('aria-valuenow', '7.4');
+  });
+
  it('renders without error', () => {
    expect(() => render(<Slider {...sliderProps} />)).not.toThrow();
  });
@@ -74,6 +107,66 @@ describe('Slider', () => {
    expect(sliderInput).toHaveValue('50');
  });

+  it('allows decimal numbers in input', async () => {
+    render(<Slider {...sliderProps} min={0} max={10} step={1} />);
+    const sliderInput = screen.getByRole('textbox');
+    const slider = screen.getByRole('slider');
+
+    await user.clear(sliderInput);
+    await user.type(sliderInput, '3.5');
+
+    expect(sliderInput).toHaveValue('3.5');
+
+    // numeric value clamped
+    expect(slider).toHaveAttribute('aria-valuenow', '4');
+  });
+
+  it('number parsing ignores non-numeric characters typed in the text input', async () => {
+    render(<Slider {...sliderProps} min={-10} max={10} step={0.1} />);
+    const sliderInput = screen.getByRole('textbox');
+    const slider = screen.getByRole('slider');
+
+    await user.clear(sliderInput);
+
+    // the characters other than numbers and the first `-` and `.` are stripped as you type
+    await user.type(sliderInput, 'ab-cd1ef.gh.1');
+
+    expect(sliderInput).toHaveValue('ab-cd1ef.gh.1');
+    expect(slider).toHaveAttribute('aria-valuenow', '-1.1');
+  });
+
+  it('number parsing allows but ignores non-numeric characters typed in the text input when step and min are integers', async () => {
+    render(<Slider {...sliderProps} min={-500} max={500} step={5} />);
+    const sliderInput = screen.getByRole('textbox');
+    const slider = screen.getByRole('slider');
+
+    await user.clear(sliderInput);
+
+    // the characters other than numbers and the first `-` and `.` are stripped as you type
+    await user.type(sliderInput, 'ab-cd1ef1gh6ij.5kl');
+
+    expect(sliderInput).toHaveValue('ab-cd1ef1gh6ij.5kl');
+
+    // value is clamped from 116 to 115
+    expect(slider).toHaveAttribute('aria-valuenow', '-115');
+  });
+
+  // this is because it's a bit confusing when the value is zeroed out and you click the input that you
+  // can't type "-" immediately and it's an easy case to handle
+  it('allows you to type "-" when the value is "0"', async () => {
+    render(<Slider {...sliderProps} min={-10} max={10} step={0.1} />);
+    const sliderInput = screen.getByRole('textbox');
+    const slider = screen.getByRole('slider');
+
+    await user.clear(sliderInput);
+
+    // the zero is stripped
+    await user.type(sliderInput, '0-1');
+
+    expect(sliderInput).toHaveValue('-1');
+    expect(slider).toHaveAttribute('aria-valuenow', '-1');
+  });
+
  it('sets value to the closest available one after blur if input value is outside of range', async () => {
    render(<Slider {...sliderProps} value={10} min={10} max={100} />);

@@ -98,4 +191,43 @@ describe('Slider', () => {
    expect(sliderInput).toHaveValue('10');
    expect(slider).toHaveAttribute('aria-valuenow', '10');
  });
+
+  // the rest of the tests are uncontrolled already, don't need to separately test that
+  it('can be a controlled input', async () => {
+    const mockOnChange = jest.fn();
+    const props: SliderProps = {
+      ...sliderProps,
+      onChange: mockOnChange,
+      min: -10,
+      max: 100,
+    };
+
+    const { rerender } = render(<Slider {...props} value={0} />);
+    const slider = screen.getByRole('slider');
+    const sliderInput = screen.getByRole('textbox');
+
+    await user.type(sliderInput, '-1');
+    // click outside the input field to blur
+    await user.click(document.body);
+
+    expect(slider).toHaveAttribute('aria-valuenow', '-1');
+    expect(sliderInput).toHaveValue('-1');
+
+    // Called once while typing "-1" (initial is "0", then "-", which is NaN and doesn't call
+    // onChange) and once more on blur
+    expect(mockOnChange).toHaveBeenCalledTimes(2);
+    expect(mockOnChange).toHaveBeenCalledWith(-1);
+
+    rerender(<Slider {...props} value={-1} />);
+
+    rerender(<Slider {...props} value={45} />);
+
+    // onChange should not be called when slider is re-rendered with a new value
+    // this check ensure the state synchronization is working properly, since accidentally
+    // causing onChange calls is a easy failure mode if that code is modified
+    expect(mockOnChange).toHaveBeenCalledTimes(2);
+
+    expect(slider).toHaveAttribute('aria-valuenow', '45');
+    expect(sliderInput).toHaveValue('45');
+  });
 });
@@ -1,7 +1,8 @@
 import { cx } from '@emotion/css';
 import { Global } from '@emotion/react';
 import SliderComponent from 'rc-slider';
-import { useState, useCallback, ChangeEvent, FocusEvent } from 'react';
+import { useState, useCallback, ChangeEvent, FocusEvent, useEffect } from 'react';
+import { usePrevious } from 'react-use';

 import { t } from '@grafana/i18n';

@@ -11,6 +12,63 @@ import { Input } from '../Input/Input';
 import { getStyles } from './styles';
 import { SliderProps } from './types';

+function stripAndParseNumber(raw: string): number {
+  const str = raw.replace(/^0+/, '');
+  let decimal = false;
+  let numericBody = '';
+  for (let i = 0; i < str.length; i += 1) {
+    const char = str.charAt(i);
+
+    // take digits
+    if (/\d/.test(char)) {
+      numericBody += char;
+    }
+    // take the first period
+    if (char === '.' && !decimal) {
+      decimal = true;
+      numericBody += '.';
+    }
+    // take only a leading negative sign
+    if (char === '-' && numericBody.length === 0) {
+      numericBody = '-';
+    }
+
+    // anything else is thrown away
+  }
+  const value = Number(numericBody);
+  return value;
+}
+
+// gets rid of pesky things like 1.20000000000000002 and such, since this needs to be printed
+// nicely for people.
+function roundFloatingPointError(n: number) {
+  return parseFloat(n.toPrecision(12));
+}
+
+function clampToAllowedValue(min: number, max: number, step: number, n: number): number {
+  // default to min
+  if (Number.isNaN(n)) {
+    return min;
+  }
+
+  // clamp to max and min
+  if (n > max) {
+    return max;
+  }
+  if (n < min) {
+    return min;
+  }
+
+  // ensure the value is exactly one of the allowed steps
+  // find the closest step
+  const closestStep = roundFloatingPointError(Math.round((n - min) / step) * step + min);
+
+  // clamp the closest found step to min/max
+  // this should never be needed unless the step isn't divisible by max-min, but it's a
+  // quick and easy check to include.
+  return Math.min(max, Math.max(min, closestStep));
+}
+
 /**
 * @public
 *
@@ -23,7 +81,7 @@ export const Slider = ({
  onAfterChange,
  orientation = 'horizontal',
  reverse,
-  step,
+  step = 1,
  value,
  ariaLabelForHandle,
  marks,
@@ -34,78 +92,83 @@ export const Slider = ({
  const isHorizontal = orientation === 'horizontal';
  const styles = useStyles2(getStyles, isHorizontal, Boolean(marks));
  const SliderWithTooltip = SliderComponent;
-  const [sliderValue, setSliderValue] = useState<number>(value ?? min);
+
+  const [inputValue, setInputValue] = useState<string>((value ?? min).toString());
+  const numericValue = clampToAllowedValue(min, max, step, stripAndParseNumber(inputValue));
+
+  // State synchronization. This is a hack since we have to maintain our own source of truth for the text input
+  const previousValue = usePrevious(value);
+  const externalValueChanged = value !== previousValue && value !== numericValue;
+  useEffect(() => {
+    if (externalValueChanged && value !== undefined) {
+      // This only causes a re-render if the value is actually different, which should
+      // only happen if the value is externally changed
+      setInputValue(String(value));
+    }
+  }, [externalValueChanged, value]);
+
  const dragHandleAriaLabel =
    ariaLabelForHandle ?? t('grafana-ui.slider.drag-handle-aria-label', 'Use arrow keys to change the value');

  const onSliderChange = useCallback(
    (v: number | number[]) => {
-      const value = typeof v === 'number' ? v : v[0];
-
-      setSliderValue(value);
-      onChange?.(value);
+      const num = typeof v === 'number' ? v : v[0];
+      setInputValue(num.toString());
+      onChange?.(num);
    },
-    [setSliderValue, onChange]
-  );
-
-  const onSliderInputChange = useCallback(
-    (e: ChangeEvent<HTMLInputElement>) => {
-      let v = +e.target.value;
-
-      if (Number.isNaN(v)) {
-        v = 0;
-      }
-
-      setSliderValue(v);
-
-      if (onChange) {
-        onChange(v);
-      }
-
-      if (onAfterChange) {
-        onAfterChange(v);
-      }
-    },
-    [onChange, onAfterChange]
-  );
-
-  // Check for min/max on input blur so user is able to enter
-  // custom values that might seem above/below min/max on first keystroke
-  const onSliderInputBlur = useCallback(
-    (e: FocusEvent<HTMLInputElement>) => {
-      const v = +e.target.value;
-
-      if (v > max) {
-        setSliderValue(max);
-      } else if (v < min) {
-        setSliderValue(min);
-      }
-    },
-    [max, min]
+    [onChange]
  );

  const handleChangeComplete = useCallback(
    (v: number | number[]) => {
-      const value = typeof v === 'number' ? v : v[0];
-      onAfterChange?.(value);
+      const num = typeof v === 'number' ? v : v[0];
+      onAfterChange?.(num);
    },
    [onAfterChange]
  );

+  const onTextInputChange = useCallback(
+    (e: ChangeEvent<HTMLInputElement>) => {
+      const raw = e.target.value;
+
+      // Update the raw input string to show what user typed, except the special case of `0-`, which
+      // should result in just `-` as a user convenience.
+      setInputValue(raw === '0-' ? '-' : raw);
+
+      // Parse and validate the number
+      const parsed = stripAndParseNumber(raw);
+      if (onChange && !Number.isNaN(parsed)) {
+        // Clamp the output value
+        onChange(clampToAllowedValue(min, max, step, parsed));
+      }
+    },
+    [onChange, min, max, step]
+  );
+
+  const onTextInputBlur = useCallback(
+    (e: FocusEvent<HTMLInputElement>) => {
+      const parsed = clampToAllowedValue(min, max, step, stripAndParseNumber(e.target.value));
+
+      // Update both numeric and string values with the clamped result
+      setInputValue(parsed.toString());
+      onChange?.(parsed);
+      onAfterChange?.(parsed);
+    },
+    [min, max, step, onChange, onAfterChange]
+  );
+
  const sliderInputClassNames = !isHorizontal ? [styles.sliderInputVertical] : [];
  const sliderInputFieldClassNames = !isHorizontal ? [styles.sliderInputFieldVertical] : [];

  return (
    <div className={cx(styles.container, styles.slider)}>
-      {/** Slider tooltip's parent component is body and therefore we need Global component to do css overrides for it. */}
      <Global styles={styles.tooltip} />
      <div className={cx(styles.sliderInput, ...sliderInputClassNames)}>
        <SliderWithTooltip
          min={min}
          max={max}
-          step={step}
-          defaultValue={value}
-          value={sliderValue}
+          step={step ?? 0.1}
+          value={numericValue}
          onChange={onSliderChange}
          onChangeComplete={handleChangeComplete}
          vertical={!isHorizontal}
@@ -120,9 +183,9 @@ export const Slider = ({
            type="text"
            width={7.5}
            className={cx(styles.sliderInputField, ...sliderInputFieldClassNames)}
-            value={sliderValue}
-            onChange={onSliderInputChange}
-            onBlur={onSliderInputBlur}
+            value={inputValue}
+            onChange={onTextInputChange}
+            onBlur={onTextInputBlur}
            min={min}
            max={max}
            id={inputId}