DEV: Rework reorder-categories modal (#25475)

app/assets/javascripts/discourse/app/components/modal/reorder-categories.hbs

@@ -1,6 +1,7 @@
 <DModal
   @title={{i18n "categories.reorder.title"}}
   @closeModal={{@closeModal}}
+  @inline={{@inline}}
   class="reorder-categories"
 >
   <:body>
@@ -10,11 +11,11 @@
         <th>{{i18n "categories.reorder.position"}}</th>
       </thead>
       <tbody>
-        {{#each this.categoriesOrdered as |category|}}
-          <tr data-category-id={{category.id}}>
+        {{#each this.sortedEntries as |entry|}}
+          <tr data-category-id={{entry.category.id}}>
             <td>
-              <div class={{concat "reorder-categories-depth-" category.depth}}>
-                {{category-badge category allowUncategorized="true"}}
+              <div class={{concat "reorder-categories-depth-" entry.depth}}>
+                {{category-badge entry.category allowUncategorized="true"}}
               </div>
             </td>
 
@@ -22,22 +23,22 @@
               <div class="reorder-categories-actions">
                 <input
                   {{on
-                    "input"
-                    (action (fn this.change category) value="target.value")
+                    "change"
+                    (action (fn this.change entry) value="target.value")
                   }}
-                  value={{category.position}}
+                  value={{entry.position}}
                   type="number"
                   min="0"
                 />
                 <DButton
-                  @action={{fn this.move category -1}}
+                  @action={{fn this.move entry -1}}
                   @icon="arrow-up"
-                  class="btn-default no-text"
+                  class="btn-default no-text move-up"
                 />
                 <DButton
-                  @action={{fn this.move category 1}}
+                  @action={{fn this.move entry 1}}
                   @icon="arrow-down"
-                  class="btn-default no-text"
+                  class="btn-default no-text move-down"
                 />
               </div>
             </td>
@@ -51,6 +52,7 @@
     <DButton
       @action={{this.save}}
       @label="categories.reorder.save"
+      @disabled={{not this.changed}}
       class="btn-primary"
     />
   </:footer>

app/assets/javascripts/discourse/app/components/modal/reorder-categories.js

@@ -1,21 +1,38 @@
-import Component from "@ember/component";
+import Component from "@glimmer/component";
+import { tracked } from "@glimmer/tracking";
 import { action } from "@ember/object";
-import { sort } from "@ember/object/computed";
-import { next } from "@ember/runloop";
 import { inject as service } from "@ember/service";
 import { ajax } from "discourse/lib/ajax";
 import { popupAjaxError } from "discourse/lib/ajax-error";
 
+class Entry {
+  @tracked position;
+
+  constructor({ position, depth, category, descendantCount }) {
+    this.position = position;
+    this.depth = depth;
+    this.category = category;
+    this.descendantCount = descendantCount;
+  }
+}
+
 export default class ReorderCategories extends Component {
   @service site;
 
-  categoriesSorting = ["position"];
+  @tracked changed = false;
+  @tracked entries = this.reorder();
 
-  @sort("site.categories", "categoriesSorting") categoriesOrdered;
+  get sortedEntries() {
+    return this.entries.sortBy("position");
+  }
 
-  init() {
-    super.init(...arguments);
-    next(() => this.reorder());
+  reorder(from) {
+    from ??= this.site.categories.map((category) => ({
+      category,
+      position: category.position,
+    }));
+
+    return this.createEntries([...from.sortBy("position")]);
   }
 
   /**
@@ -30,128 +47,109 @@ export default class ReorderCategories extends Component {
    *      other                 parent/c2/c1
    *      parent/c2             other
    **/
-  reorder() {
-    this.reorderChildren(null, 0, 0);
-  }
+  createEntries(from, position = 0, categoryId = null, depth = 0) {
+    let result = [];
 
-  reorderChildren(categoryId, depth, index) {
-    for (const category of this.categoriesOrdered) {
+    for (const entry of from) {
       if (
-        (categoryId === null && !category.get("parent_category_id")) ||
-        category.get("parent_category_id") === categoryId
+        (categoryId === null && !entry.category.parent_category_id) ||
+        entry.category.parent_category_id === categoryId
       ) {
-        category.setProperties({ depth, position: index++ });
-        index = this.reorderChildren(category.get("id"), depth + 1, index);
+        const descendants = this.createEntries(
+          from,
+          position + result.length + 1,
+          entry.category.id,
+          depth + 1
+        );
+
+        result = [
+          ...result,
+          new Entry({
+            position: position + result.length,
+            depth,
+            category: entry.category,
+            descendantCount: descendants.length,
+          }),
+          ...descendants,
+        ];
       }
     }
 
-    return index;
-  }
-
-  countDescendants(category) {
-    if (!category.get("subcategories")) {
-      return 0;
-    }
-
-    return category
-      .get("subcategories")
-      .reduce(
-        (count, subcategory) => count + this.countDescendants(subcategory),
-        category.get("subcategories").length
-      );
+    return result;
   }
 
   @action
-  move(category, direction) {
-    let targetPosition = category.get("position") + direction;
+  move(entry, delta) {
+    let targetPosition = entry.position + delta;
 
     // Adjust target position for sub-categories
-    if (direction > 0) {
+    if (delta > 0) {
       // Moving down (position gets larger)
-      if (category.get("isParent")) {
+      if (entry.descendantCount) {
         // This category has subcategories, adjust targetPosition to account for them
-        let offset = this.countDescendants(category);
-        if (direction <= offset) {
+        if (entry.descendantCount >= delta) {
           // Only apply offset if target position is occupied by a subcategory
           // Seems weird but fixes a UX quirk
-          targetPosition += offset;
+          targetPosition += entry.descendantCount;
         }
       }
     } else {
       // Moving up (position gets smaller)
-      const otherCategory = this.categoriesOrdered.find(
-        (c) =>
-          // find category currently at targetPosition
-          c.get("position") === targetPosition
-      );
-      if (otherCategory && otherCategory.get("ancestors")) {
+      const ancestors = this.sortedEntries[targetPosition]?.category?.ancestors;
+      if (ancestors) {
         // Target category is a subcategory, adjust targetPosition to account for ancestors
-        const highestAncestor = otherCategory
-          .get("ancestors")
-          .reduce((current, min) =>
-            current.get("position") < min.get("position") ? current : min
-          );
-        targetPosition = highestAncestor.get("position");
+        const highestAncestorEntry = this.sortedEntries.findBy(
+          "category.id",
+          ancestors[0].id
+        );
+        targetPosition = highestAncestorEntry.position;
       }
     }
 
     // Adjust target position for range bounds
-    if (targetPosition >= this.categoriesOrdered.length) {
+    if (targetPosition >= this.entries.length) {
       // Set to max
-      targetPosition = this.categoriesOrdered.length - 1;
+      targetPosition = this.entries.length - 1;
     } else if (targetPosition < 0) {
       // Set to min
       targetPosition = 0;
     }
 
     // Update other categories between current and target position
-    for (const c of this.categoriesOrdered) {
-      if (direction < 0) {
-        // Moving up (position gets smaller)
-        if (
-          c.get("position") < category.get("position") &&
-          c.get("position") >= targetPosition
-        ) {
-          const newPosition = c.get("position") + 1;
-          c.set("position", newPosition);
+    for (const e of this.sortedEntries) {
+      if (delta > 0) {
+        // Moving down (position gets larger)
+        if (e.position > entry.position && e.position <= targetPosition) {
+          e.position -= 1;
         }
       } else {
-        // Moving down (position gets larger)
-        if (
-          c.get("position") > category.get("position") &&
-          c.get("position") <= targetPosition
-        ) {
-          const newPosition = c.get("position") - 1;
-          c.set("position", newPosition);
+        // Moving up (position gets smaller)
+        if (e.position < entry.position && e.position >= targetPosition) {
+          e.position += 1;
         }
       }
     }
 
     // Update this category's position to target position
-    category.set("position", targetPosition);
+    entry.position = targetPosition;
 
-    this.reorder();
+    this.entries = this.reorder(this.sortedEntries);
+    this.changed = true;
   }
 
   @action
-  change(category, newPosition) {
-    newPosition = parseInt(newPosition, 10);
-    newPosition =
-      newPosition < category.get("position")
-        ? Math.ceil(newPosition)
-        : Math.floor(newPosition);
-
-    const direction = newPosition - category.get("position");
-    this.move(category, direction);
+  change(entry, newPosition) {
+    const delta = parseInt(newPosition, 10) - entry.position;
+    this.move(entry, delta);
   }
 
   @action
   async save() {
-    this.reorder();
+    const entries = this.reorder(this.sortedEntries);
 
     const data = {};
-    for (const category of this.site.categories) {
-      data[category.get("id")] = category.get("position");
+    for (const { category, position } of entries) {
+      data[category.id] = position;
     }
 
     try {

app/assets/javascripts/discourse/tests/integration/components/reorder-categories-test.gjs

@@ -0,0 +1,171 @@
+import { getOwner } from "@ember/application";
+import { click, fillIn, render } from "@ember/test-helpers";
+import { module, test } from "qunit";
+import ReorderCategories from "discourse/components/modal/reorder-categories";
+import { setupRenderingTest } from "discourse/tests/helpers/component-test";
+
+module("Integration | Component | ReorderCategories", function (hooks) {
+  setupRenderingTest(hooks);
+
+  test("shows categories in order", async function (assert) {
+    const store = getOwner(this).lookup("service:store");
+    const site = getOwner(this).lookup("service:site");
+    site.set("categories", [
+      store.createRecord("category", { id: 1, position: 0 }),
+      store.createRecord("category", { id: 2, position: 0 }),
+      store.createRecord("category", { id: 3, position: 0 }),
+    ]);
+
+    await render(<template><ReorderCategories @inline={{true}} /></template>);
+
+    assert.dom("tr:nth-child(1)").hasAttribute("data-category-id", "1");
+    assert.dom("tr:nth-child(2)").hasAttribute("data-category-id", "2");
+    assert.dom("tr:nth-child(3)").hasAttribute("data-category-id", "3");
+  });
+
+  test("reorders subcategories after their parent categories, while maintaining the relative order", async function (assert) {
+    const store = getOwner(this).lookup("service:store");
+    const parent = store.createRecord("category", {
+      id: 1,
+      position: 1,
+      name: "parent",
+    });
+    const child1 = store.createRecord("category", {
+      id: 2,
+      position: 3,
+      name: "child1",
+      parent_category_id: 1,
+    });
+    const child2 = store.createRecord("category", {
+      id: 3,
+      position: 0,
+      name: "child2",
+      parent_category_id: 1,
+    });
+    const other = store.createRecord("category", {
+      id: 4,
+      position: 2,
+      name: "other",
+    });
+    const site = getOwner(this).lookup("service:site");
+    site.set("categories", [child2, parent, other, child1]);
+
+    await render(<template><ReorderCategories @inline={{true}} /></template>);
+
+    assert.dom("tr:nth-child(1) .badge-category__name").hasText("parent");
+    assert.dom("tr:nth-child(2) .badge-category__name").hasText("child2");
+    assert.dom("tr:nth-child(3) .badge-category__name").hasText("child1");
+    assert.dom("tr:nth-child(4) .badge-category__name").hasText("other");
+  });
+
+  test("changing the position number of a category should place it at given position", async function (assert) {
+    const store = getOwner(this).lookup("service:store");
+    const foo = store.createRecord("category", {
+      id: 1,
+      position: 0,
+      name: "foo",
+    });
+    const bar = store.createRecord("category", {
+      id: 2,
+      position: 1,
+      name: "bar",
+    });
+    const baz = store.createRecord("category", {
+      id: 3,
+      position: 2,
+      name: "baz",
+    });
+    const site = getOwner(this).lookup("service:site");
+    site.set("categories", [foo, bar, baz]);
+
+    await render(<template><ReorderCategories @inline={{true}} /></template>);
+
+    // Move category 'foo' from position 0 to position 2
+    await fillIn("tr:nth-child(1) input", "2");
+
+    assert.dom("tr:nth-child(1) .badge-category__name").hasText("bar");
+    assert.dom("tr:nth-child(2) .badge-category__name").hasText("baz");
+    assert.dom("tr:nth-child(3) .badge-category__name").hasText("foo");
+  });
+
+  test("changing the position number of a category should place it at given position and respect children", async function (assert) {
+    const store = getOwner(this).lookup("service:store");
+    const foo = store.createRecord("category", {
+      id: 1,
+      position: 0,
+      name: "foo",
+    });
+    const fooChild = store.createRecord("category", {
+      id: 4,
+      position: 1,
+      name: "foo-child",
+      parent_category_id: 1,
+    });
+    const bar = store.createRecord("category", {
+      id: 2,
+      position: 2,
+      name: "bar",
+    });
+    const baz = store.createRecord("category", {
+      id: 3,
+      position: 3,
+      name: "baz",
+    });
+    const site = getOwner(this).lookup("service:site");
+    site.set("categories", [foo, fooChild, bar, baz]);
+
+    await render(<template><ReorderCategories @inline={{true}} /></template>);
+    await fillIn("tr:nth-child(1) input", "3");
+
+    assert.dom("tr:nth-child(1) .badge-category__name").hasText("bar");
+    assert.dom("tr:nth-child(2) .badge-category__name").hasText("baz");
+    assert.dom("tr:nth-child(3) .badge-category__name").hasText("foo");
+    assert.dom("tr:nth-child(4) .badge-category__name").hasText("foo-child");
+  });
+
+  test("changing the position through click on arrow of a category should place it at given position and respect children", async function (assert) {
+    const store = getOwner(this).lookup("service:store");
+    const fooChildChild = store.createRecord("category", {
+      id: 105,
+      position: 2,
+      name: "foo-child-child",
+      parent_category_id: 104,
+    });
+    const fooChild = store.createRecord("category", {
+      id: 104,
+      position: 1,
+      name: "foo-child",
+      parent_category_id: 101,
+      subcategories: [fooChildChild],
+    });
+    const foo = store.createRecord("category", {
+      id: 101,
+      position: 0,
+      name: "foo",
+      subcategories: [fooChild],
+    });
+    const bar = store.createRecord("category", {
+      id: 102,
+      position: 3,
+      name: "bar",
+    });
+    const baz = store.createRecord("category", {
+      id: 103,
+      position: 4,
+      name: "baz",
+    });
+    const site = getOwner(this).lookup("service:site");
+    site.set("categories", [foo, fooChild, fooChildChild, bar, baz]);
+
+    await render(<template><ReorderCategories @inline={{true}} /></template>);
+    await click("tr:nth-child(1) button.move-down");
+
+    assert.dom("tr:nth-child(1) .badge-category__name").hasText("bar");
+    assert.dom("tr:nth-child(2) .badge-category__name").hasText("foo");
+    assert.dom("tr:nth-child(3) .badge-category__name").hasText("foo-child");
+    assert
+      .dom("tr:nth-child(4) .badge-category__name")
+      .hasText("foo-child-child");
+    assert.dom("tr:nth-child(5) .badge-category__name").hasText("baz");
+  });
+});

app/assets/javascripts/discourse/tests/unit/components/reorder-categories-test.js

@@ -1,205 +0,0 @@
-import { getOwner } from "@ember/application";
-import { setupTest } from "ember-qunit";
-import { module, test } from "qunit";
-
-module("Unit | Component | reorder-categories", function (hooks) {
-  setupTest(hooks);
-
-  test("reorder set unique position number", function (assert) {
-    const component = this.owner
-      .factoryFor("component:modal/reorder-categories")
-      .create();
-    const store = getOwner(this).lookup("service:store");
-
-    const site = getOwner(this).lookup("service:site");
-    site.set("categories", [
-      store.createRecord("category", { id: 1, position: 0 }),
-      store.createRecord("category", { id: 2, position: 0 }),
-      store.createRecord("category", { id: 3, position: 0 }),
-    ]);
-
-    component.reorder();
-
-    component.categoriesOrdered.forEach((category, index) => {
-      assert.strictEqual(category.get("position"), index);
-    });
-  });
-
-  test("reorder places subcategories after their parent categories, while maintaining the relative order", function (assert) {
-    const component = this.owner
-      .factoryFor("component:modal/reorder-categories")
-      .create();
-    const store = getOwner(this).lookup("service:store");
-
-    const parent = store.createRecord("category", {
-      id: 1,
-      position: 1,
-      slug: "parent",
-    });
-    const child1 = store.createRecord("category", {
-      id: 2,
-      position: 3,
-      slug: "child1",
-      parent_category_id: 1,
-    });
-    const child2 = store.createRecord("category", {
-      id: 3,
-      position: 0,
-      slug: "child2",
-      parent_category_id: 1,
-    });
-    const other = store.createRecord("category", {
-      id: 4,
-      position: 2,
-      slug: "other",
-    });
-
-    const expectedOrderSlugs = ["parent", "child2", "child1", "other"];
-    const site = getOwner(this).lookup("service:site");
-    site.set("categories", [child2, parent, other, child1]);
-
-    component.reorder();
-
-    assert.deepEqual(
-      component.categoriesOrdered.mapBy("slug"),
-      expectedOrderSlugs
-    );
-  });
-
-  test("changing the position number of a category should place it at given position", function (assert) {
-    const component = this.owner
-      .factoryFor("component:modal/reorder-categories")
-      .create();
-    const store = getOwner(this).lookup("service:store");
-
-    const elem1 = store.createRecord("category", {
-      id: 1,
-      position: 0,
-      slug: "foo",
-    });
-
-    const elem2 = store.createRecord("category", {
-      id: 2,
-      position: 1,
-      slug: "bar",
-    });
-
-    const elem3 = store.createRecord("category", {
-      id: 3,
-      position: 2,
-      slug: "test",
-    });
-
-    const site = getOwner(this).lookup("service:site");
-    site.set("categories", [elem1, elem2, elem3]);
-
-    // Move category 'foo' from position 0 to position 2
-    component.change(elem1, "2");
-
-    assert.deepEqual(component.categoriesOrdered.mapBy("slug"), [
-      "bar",
-      "test",
-      "foo",
-    ]);
-  });
-
-  test("changing the position number of a category should place it at given position and respect children", function (assert) {
-    const component = this.owner
-      .factoryFor("component:modal/reorder-categories")
-      .create();
-    const store = getOwner(this).lookup("service:store");
-
-    const elem1 = store.createRecord("category", {
-      id: 1,
-      position: 0,
-      slug: "foo",
-    });
-
-    const child1 = store.createRecord("category", {
-      id: 4,
-      position: 1,
-      slug: "foo-child",
-      parent_category_id: 1,
-    });
-
-    const elem2 = store.createRecord("category", {
-      id: 2,
-      position: 2,
-      slug: "bar",
-    });
-
-    const elem3 = store.createRecord("category", {
-      id: 3,
-      position: 3,
-      slug: "test",
-    });
-
-    const site = getOwner(this).lookup("service:site");
-    site.set("categories", [elem1, child1, elem2, elem3]);
-
-    component.change(elem1, "3");
-
-    assert.deepEqual(component.categoriesOrdered.mapBy("slug"), [
-      "bar",
-      "test",
-      "foo",
-      "foo-child",
-    ]);
-  });
-
-  test("changing the position through click on arrow of a category should place it at given position and respect children", function (assert) {
-    const component = this.owner
-      .factoryFor("component:modal/reorder-categories")
-      .create();
-    const store = getOwner(this).lookup("service:store");
-
-    const child2 = store.createRecord("category", {
-      id: 105,
-      position: 2,
-      slug: "foo-child-child",
-      parent_category_id: 104,
-    });
-
-    const child1 = store.createRecord("category", {
-      id: 104,
-      position: 1,
-      slug: "foo-child",
-      parent_category_id: 101,
-      subcategories: [child2],
-    });
-
-    const elem1 = store.createRecord("category", {
-      id: 101,
-      position: 0,
-      slug: "foo",
-      subcategories: [child1],
-    });
-
-    const elem2 = store.createRecord("category", {
-      id: 102,
-      position: 3,
-      slug: "bar",
-    });
-
-    const elem3 = store.createRecord("category", {
-      id: 103,
-      position: 4,
-      slug: "test",
-    });
-
-    const site = getOwner(this).lookup("service:site");
-    site.set("categories", [elem1, child1, child2, elem2, elem3]);
-
-    component.reorder();
-
-    component.move(elem1, 1);
-
-    assert.deepEqual(component.categoriesOrdered.mapBy("slug"), [
-      "bar",
-      "foo",
-      "foo-child",
-      "foo-child-child",
-      "test",
-    ]);
-  });
-});

app/assets/stylesheets/common/base/reorder-categories.scss

@@ -4,16 +4,12 @@
       padding-bottom: 0.5em;
     }
   }
-  input[type="text"] {
-    margin: 0;
-    max-width: 2.5em;
-    padding: 0.35em;
-    text-align: center;
 
-    @include breakpoint(mobile-extra-large) {
-      width: 2em;
-    }
+  input[type="number"] {
+    margin: 0;
+    max-width: 4em;
   }
+
   table {
     padding-bottom: 150px;
     margin: 0 0.667em;
@@ -25,6 +21,7 @@
       }
     }
   }
+
   .badge-category__wrapper .badge-category {
     max-width: 20em;
     @include breakpoint(mobile-extra-large) {