Add logic to fix out-of-bounds count values in css measure when switc…

…hing units
rstudio · Sep 14, 2023 · a3f385a · a3f385a
1 parent 4dd61ae
commit a3f385a
Show file tree

Hide file tree

Showing 2 changed files with 45 additions and 28 deletions.
diff --git a/inst/editor/src/components/Inputs/CSSUnitInput/CSSMeasure.ts b/inst/editor/src/components/Inputs/CSSUnitInput/CSSMeasure.ts
@@ -33,7 +33,7 @@ export const infoForUnits: Record<
   auto: { defaultCount: 0, step: 1, min: 0, max: 0 },
 };
 
-type ParsedCSSMeasure =
+export type ParsedCSSMeasure =
   | { count: number; unit: CSSUnit }
   | { count: null; unit: "auto" };
 
@@ -68,3 +68,31 @@ export function deparseCSSMeasure(parsed: ParsedCSSMeasure): CSSMeasure {
   if (parsed.unit === "auto") return "auto";
   return `${parsed.count}${parsed.unit}`;
 }
+
+/**
+ * Takes a count value and makes sure it fits within the allowed bounds for the given unit
+ * @param count Count for current measure. If null, will return the default count for the unit
+ * @param newUnit Unit for current measure
+ * @returns The count value
+ */
+export function validateCountAfterUnitChange(
+  count: number | null,
+  newUnit: CSSUnitWAuto
+): ParsedCSSMeasure {
+  if (newUnit === "auto") {
+    return { count: null, unit: "auto" };
+  }
+
+  const unitInfo = infoForUnits[newUnit];
+
+  const validCount =
+    count === null
+      ? unitInfo.defaultCount
+      : count < unitInfo.min
+      ? unitInfo.min
+      : count > unitInfo.max
+      ? unitInfo.max
+      : count;
+
+  return { count: validCount, unit: newUnit };
+}
diff --git a/inst/editor/src/components/Inputs/CSSUnitInput/CSSUnitInput.tsx b/inst/editor/src/components/Inputs/CSSUnitInput/CSSUnitInput.tsx
@@ -5,9 +5,9 @@ import { makeLabelId } from "../../../ui-node-definitions/inputFieldTypes";
 import { mergeClasses } from "../../../utils/mergeClasses";
 import { NumberInputSimple } from "../NumberInput/NumberInput";
 
-import type { CSSUnitWAuto } from "./CSSMeasure";
-import { infoForUnits } from "./CSSMeasure";
-import { deparseCSSMeasure, parseCSSMeasure } from "./CSSMeasure";
+import type { CSSUnitWAuto, ParsedCSSMeasure } from "./CSSMeasure";
+import { validateCountAfterUnitChange } from "./CSSMeasure";
+import { deparseCSSMeasure, infoForUnits, parseCSSMeasure } from "./CSSMeasure";
 import { CSSUnitChooser } from "./CSSUnitChooser";
 import classes from "./CSSUnitInput.module.css";
 
@@ -20,53 +20,42 @@ export function CSSUnitInput({
 }: InputComponentByType<"cssMeasure">) {
   const { count, unit } = parseCSSMeasure(initialValue);
 
+  // Allow us to pass the object-form of the css measure to on change to keep
+  // code cleaner
+  const updateValue = React.useCallback(
+    (x: ParsedCSSMeasure) => onChange(deparseCSSMeasure(x)),
+    [onChange]
+  );
+
   const updateCount = React.useCallback(
     (newCount?: number) => {
       if (newCount === undefined) {
         if (unit !== "auto") {
           throw new Error("Undefined count with auto units");
         }
 
-        onChange(deparseCSSMeasure({ unit, count: null }));
+        updateValue({ unit, count: null });
         return;
       }
+
       if (unit === "auto") {
         // eslint-disable-next-line no-console
         console.error("How did you change the count of an auto unit?");
         return;
       }
 
-      onChange(deparseCSSMeasure({ unit, count: newCount }));
+      updateValue({ unit, count: newCount });
     },
-    [onChange, unit]
+    [unit, updateValue]
   );
 
   const updateUnit = React.useCallback(
     (newUnit: CSSUnitWAuto) => {
-      if (newUnit === "auto") {
-        onChange(
-          deparseCSSMeasure({
-            unit: newUnit,
-            count: null,
-          })
-        );
-        return;
-      }
-
-      if (unit === "auto") {
-        onChange(
-          deparseCSSMeasure({
-            unit: newUnit,
-            count: infoForUnits[newUnit].defaultCount,
-          })
-        );
-        return;
-      }
+      updateValue(validateCountAfterUnitChange(count, newUnit));
 
-      onChange(deparseCSSMeasure({ unit: newUnit, count: count }));
       return;
     },
-    [count, onChange, unit]
+    [count, updateValue]
   );
 
   if (!units.includes(unit)) {