From 3e13a06a8f797b4e3cce8735e89e2a09197aeda7 Mon Sep 17 00:00:00 2001
From: Andrew Kingston <andrew@kingston.dev>
Date: Thu, 23 May 2024 12:10:53 +0100
Subject: [PATCH] Update option auto naming

---
 .../DataTable/modals/OptionsEditor.svelte     | 188 ++++++++++--------
 packages/builder/src/helpers/duplicate.js     |  11 +-
 .../src/helpers/tests/duplicate.test.js       |  48 +++--
 3 files changed, 147 insertions(+), 100 deletions(-)

diff --git a/packages/builder/src/components/backend/DataTable/modals/OptionsEditor.svelte b/packages/builder/src/components/backend/DataTable/modals/OptionsEditor.svelte
index 26d4e39c36..ed66906aea 100644
--- a/packages/builder/src/components/backend/DataTable/modals/OptionsEditor.svelte
+++ b/packages/builder/src/components/backend/DataTable/modals/OptionsEditor.svelte
@@ -4,6 +4,7 @@
   import { Icon, Popover } from "@budibase/bbui"
   import { onMount, tick } from "svelte"
   import { Constants } from "@budibase/frontend-core"
+  import { getSequentialName } from "helpers/duplicate"
 
   export let constraints
   export let optionColors = {}
@@ -15,25 +16,33 @@
   let colorPopovers = []
   let anchors = []
 
-  const removeInput = idx => {
-    delete optionColors[options[idx].name]
-    constraints.inclusion = constraints.inclusion.filter((e, i) => i !== idx)
-    options = options.filter((e, i) => i !== idx)
+  $: enrichedOptions = options.map((option, idx) => ({
+    ...option,
+    color: optionColors?.[option.name] || defaultColor(idx),
+  }))
+
+  const defaultColor = idx => OptionColours[idx % OptionColours.length]
+
+  const removeInput = name => {
+    delete optionColors[name]
+    constraints.inclusion = constraints.inclusion.filter(opt => opt !== name)
+    options = options.filter(opt => opt.name !== name)
     colorPopovers.pop(undefined)
     anchors.pop(undefined)
   }
 
   const addNewInput = async () => {
-    const newName = `Option ${constraints.inclusion.length + 1}`
-    const id = Math.random()
-    options = [...options, { name: newName, id }]
+    const newName = getSequentialName(constraints.inclusion, "Option ", {
+      numberFirstItem: true,
+    })
+    const newId = Math.random()
+    options = [...options, { name: newName, id: newId }]
     constraints.inclusion = [...constraints.inclusion, newName]
-    optionColors[newName] =
-      OptionColours[(options.length - 1) % OptionColours.length]
+    optionColors[newName] = defaultColor(options.length - 1)
     colorPopovers.push(undefined)
     anchors.push(undefined)
     await tick()
-    document.getElementById(`option-${id}`)?.focus()
+    document.getElementById(`option-${newId}`)?.focus()
   }
 
   const handleDndConsider = e => {
@@ -44,16 +53,29 @@
     constraints.inclusion = options.map(option => option.name)
   }
 
-  const handleColorChange = (optionName, color, idx) => {
-    optionColors[optionName] = color
+  const handleColorChange = (name, color, idx) => {
+    optionColors[name] = color
     colorPopovers[idx].hide()
   }
 
-  const handleNameChange = (optionName, idx, value) => {
-    constraints.inclusion[idx] = value
-    options[idx].name = value
-    optionColors[value] = optionColors[optionName]
-    delete optionColors[optionName]
+  const handleNameChange = (name, idx, newName) => {
+    // Check we don't already have this option
+    const existing = options.some((option, optionIdx) => {
+      return newName === option.name && idx !== optionIdx
+    })
+    const invalid = !newName || existing
+    options.find(option => option.name === name).invalid = invalid
+    options = options.slice()
+
+    // Stop if invalid or no change
+    if (invalid || name === newName) {
+      return
+    }
+
+    constraints.inclusion[idx] = newName
+    options[idx].name = newName
+    optionColors[newName] = optionColors[name]
+    delete optionColors[name]
   }
 
   const openColorPickerPopover = optionIdx => {
@@ -66,79 +88,84 @@
     }
   }
 
-  const getOptionColor = (name, idx) => {
-    return optionColors?.[name] || OptionColours[idx % OptionColours.length]
-  }
-
   onMount(() => {
     // Initialize anchor arrays on mount, assuming 'options' is already populated
     colorPopovers = constraints.inclusion.map(() => undefined)
     anchors = constraints.inclusion.map(() => undefined)
-
     options = constraints.inclusion.map(value => ({
-      name: value,
       id: Math.random(),
+      name: value,
     }))
   })
 </script>
 
 <!-- svelte-ignore a11y-no-static-element-interactions -->
 <!-- svelte-ignore a11y-click-events-have-key-events -->
-<div
-  class="options"
-  use:dndzone={{
-    items: options,
-    flipDurationMs,
-    dropTargetStyle: { outline: "none" },
-  }}
-  on:consider={handleDndConsider}
-  on:finalize={handleDndFinalize}
->
-  {#each options as option, idx (`${option.id}-${idx}`)}
-    {@const color = getOptionColor(option.name, idx)}
-    <div class="option" animate:flip={{ duration: flipDurationMs }}>
-      <div class="drag-handle">
-        <Icon name="DragHandle" size="L" />
-      </div>
+<div class="wrapper">
+  <div
+    class="options"
+    use:dndzone={{
+      items: options,
+      flipDurationMs,
+      dropTargetStyle: { outline: "none" },
+    }}
+    on:consider={handleDndConsider}
+    on:finalize={handleDndFinalize}
+  >
+    {#each enrichedOptions as option, idx (option.id)}
       <div
-        bind:this={anchors[idx]}
-        class="color-picker"
-        on:click={e => openColorPickerPopover(idx, e.target)}
+        class="option"
+        animate:flip={{ duration: flipDurationMs }}
+        class:invalid={option.invalid}
       >
-        <div class="circle" style="--color:{color}">
-          <Popover
-            bind:this={colorPopovers[idx]}
-            anchor={anchors[idx]}
-            align="left"
-            offset={0}
-            animate={false}
-            resizable={false}
-          >
-            <div class="colors" data-ignore-click-outside="true">
-              {#each OptionColours as colorOption}
-                <div
-                  on:click={() =>
-                    handleColorChange(option.name, colorOption, idx)}
-                  style="--color:{colorOption};"
-                  class="circle"
-                  class:selected={colorOption === color}
-                />
-              {/each}
-            </div>
-          </Popover>
+        <div class="drag-handle">
+          <Icon name="DragHandle" size="L" />
         </div>
+        <div
+          bind:this={anchors[idx]}
+          class="color-picker"
+          on:click={e => openColorPickerPopover(idx, e.target)}
+        >
+          <div class="circle" style="--color:{option.color}">
+            <Popover
+              bind:this={colorPopovers[idx]}
+              anchor={anchors[idx]}
+              align="left"
+              offset={0}
+              animate={false}
+              resizable={false}
+            >
+              <div class="colors" data-ignore-click-outside="true">
+                {#each OptionColours as colorOption}
+                  <div
+                    on:click={() =>
+                      handleColorChange(option.name, colorOption, idx)}
+                    style="--color:{colorOption};"
+                    class="circle"
+                    class:selected={colorOption === option.color}
+                  />
+                {/each}
+              </div>
+            </Popover>
+          </div>
+        </div>
+        <input
+          class="option-name"
+          type="text"
+          value={option.name}
+          placeholder="Option name"
+          id="option-{option.id}"
+          on:input={e => handleNameChange(option.name, idx, e.target.value)}
+        />
+        <Icon
+          name="Close"
+          hoverable
+          size="S"
+          on:click={removeInput(option.name)}
+        />
       </div>
-      <input
-        class="option-name"
-        type="text"
-        on:change={e => handleNameChange(option.name, idx, e.target.value)}
-        value={option.name}
-        placeholder="Option name"
-        id="option-{option.id}"
-      />
-      <Icon name="Close" hoverable size="S" on:click={removeInput(idx)} />
-    </div>
-  {/each}
+    {/each}
+  </div>
   <div on:click={addNewInput} class="add-option">
     <Icon name="Add" />
     <div>Add option</div>
@@ -147,13 +174,14 @@
 
 <style>
   /* Container */
-  .options {
+  .wrapper {
     overflow: hidden;
     border-radius: 4px;
     border: 1px solid var(--spectrum-global-color-gray-300);
     background-color: var(--spectrum-global-color-gray-50);
   }
-  .options > * {
+  .options > *,
+  .add-option {
     height: 32px;
   }
 
@@ -164,12 +192,16 @@
     display: flex;
     flex-direction: row;
     align-items: center;
+    border: 1px solid transparent;
     border-bottom: 1px solid var(--spectrum-global-color-gray-300);
     gap: var(--spacing-m);
     padding: 0 var(--spacing-m) 0 var(--spacing-s);
   }
-  .option:hover,
-  .option:focus {
+  .option.invalid {
+    border: 1px solid var(--spectrum-global-color-red-400);
+  }
+  .option:not(.invalid):hover,
+  .option:not(.invalid):focus {
     background: var(--spectrum-global-color-gray-100);
   }
 
diff --git a/packages/builder/src/helpers/duplicate.js b/packages/builder/src/helpers/duplicate.js
index c2a924f97b..361e1faa25 100644
--- a/packages/builder/src/helpers/duplicate.js
+++ b/packages/builder/src/helpers/duplicate.js
@@ -70,13 +70,18 @@ export const duplicateName = (name, allNames) => {
  * @param getName optional function to extract the name for an item, if not a
  *  flat array of strings
  */
-export const getSequentialName = (items, prefix, getName = x => x) => {
+export const getSequentialName = (
+  items,
+  prefix,
+  { getName = x => x, numberFirstItem = false } = {}
+) => {
   if (!prefix?.length || !getName) {
     return null
   }
   const trimmedPrefix = prefix.trim()
+  const firstName = numberFirstItem ? `${prefix}1` : trimmedPrefix
   if (!items?.length) {
-    return trimmedPrefix
+    return firstName
   }
   let max = 0
   items.forEach(item => {
@@ -96,5 +101,5 @@ export const getSequentialName = (items, prefix, getName = x => x) => {
       max = num
     }
   })
-  return max === 0 ? trimmedPrefix : `${prefix}${max + 1}`
+  return max === 0 ? firstName : `${prefix}${max + 1}`
 }
diff --git a/packages/builder/src/helpers/tests/duplicate.test.js b/packages/builder/src/helpers/tests/duplicate.test.js
index 7e51c5ff2a..131e76a6c2 100644
--- a/packages/builder/src/helpers/tests/duplicate.test.js
+++ b/packages/builder/src/helpers/tests/duplicate.test.js
@@ -43,61 +43,71 @@ describe("duplicate", () => {
 
 describe("getSequentialName", () => {
   it("handles nullish items", async () => {
-    const name = getSequentialName(null, "foo", () => {})
+    const name = getSequentialName(null, "foo")
     expect(name).toBe("foo")
   })
 
   it("handles nullish prefix", async () => {
-    const name = getSequentialName([], null, () => {})
-    expect(name).toBe(null)
-  })
-
-  it("handles nullish getName function", async () => {
-    const name = getSequentialName([], "foo", null)
+    const name = getSequentialName([], null)
     expect(name).toBe(null)
   })
 
   it("handles just the prefix", async () => {
-    const name = getSequentialName(["foo"], "foo", x => x)
+    const name = getSequentialName(["foo"], "foo")
     expect(name).toBe("foo2")
   })
 
   it("handles continuous ranges", async () => {
-    const name = getSequentialName(["foo", "foo2", "foo3"], "foo", x => x)
+    const name = getSequentialName(["foo", "foo2", "foo3"], "foo")
     expect(name).toBe("foo4")
   })
 
   it("handles discontinuous ranges", async () => {
-    const name = getSequentialName(["foo", "foo3"], "foo", x => x)
+    const name = getSequentialName(["foo", "foo3"], "foo")
     expect(name).toBe("foo4")
   })
 
   it("handles a space inside the prefix", async () => {
-    const name = getSequentialName(["foo", "foo 2", "foo 3"], "foo ", x => x)
+    const name = getSequentialName(["foo", "foo 2", "foo 3"], "foo ")
     expect(name).toBe("foo 4")
   })
 
   it("handles a space inside the prefix with just the prefix", async () => {
-    const name = getSequentialName(["foo"], "foo ", x => x)
+    const name = getSequentialName(["foo"], "foo ")
     expect(name).toBe("foo 2")
   })
 
   it("handles no matches", async () => {
-    const name = getSequentialName(["aaa", "bbb"], "foo", x => x)
+    const name = getSequentialName(["aaa", "bbb"], "foo")
     expect(name).toBe("foo")
   })
 
   it("handles similar names", async () => {
-    const name = getSequentialName(
-      ["fooo1", "2foo", "a3foo4", "5foo5"],
-      "foo",
-      x => x
-    )
+    const name = getSequentialName(["fooo1", "2foo", "a3foo4", "5foo5"], "foo")
     expect(name).toBe("foo")
   })
 
   it("handles non-string names", async () => {
-    const name = getSequentialName([null, 4123, [], {}], "foo", x => x)
+    const name = getSequentialName([null, 4123, [], {}], "foo")
     expect(name).toBe("foo")
   })
+
+  it("handles deep getters", async () => {
+    const name = getSequentialName([{ a: "foo 1" }], "foo ", {
+      getName: x => x.a,
+    })
+    expect(name).toBe("foo 2")
+  })
+
+  it("handles a mixture of spaces and not", async () => {
+    const name = getSequentialName(["foo", "foo 1", "foo 2"], "foo")
+    expect(name).toBe("foo3")
+  })
+
+  it("handles numbering the first item", async () => {
+    const name = getSequentialName(["foo1", "foo2", "foo"], "foo ", {
+      numberFirstItem: true,
+    })
+    expect(name).toBe("foo 3")
+  })
 })