UX: Improvements for reordering categories (#13013)

UX: Improvements for reordering categories (#13013)

  • UX: Improvements to reorder categories UX

Before, moving a category from, for example, position 25 to position 0 would result in switching the positions of the two categories at those positions.

Category A at position 0 would move to position 25, and Category B at position 25 would move to position 0.

Instead of switching positions, the reorder categories function should retain the order of categories except for the one being moved.

So, Category B at position 25 would still move to position 0, but Category A is merely bumped down to position 1.

This improves the UX because if a user really wants to switch the two categories, it results in one extra step. However in the other (what I think is normal) case, it saves the 24 other switches the user has to make to get Category A back to position 1 (you can imagine the user having to click the up arrow button repeatedly to return Category A to the top of the page). Now, imagine trying to do this with a site with 100s of categories. Yikes!

The UX improvement described above is what this commit accomplishes by redesigning the move() method of the reorder-categories controller. It adds some overhead to adjust the positions of all categories in between the origin and target positions, but in testing this is not noticible to the user. It’s better for the computer to do extra work than the user.

  • UX: Allow decimal input in reorder-categories for more precise positioning.

A common UX pattern when reordering a list of items is to allow a user to specify a target position as a decimal between two valid integer positions. The user is indicating they want the target list item to move in between the list items at the positions on either side of the target position.

For example, say there are three categories Category A at position 0, Category B at position 1, and Category C at position 3.

To move Category C in between Categories A and B, a user can now simply update Category C’s position to 0.5.

diff --git a/app/assets/javascripts/discourse/app/controllers/reorder-categories.js b/app/assets/javascripts/discourse/app/controllers/reorder-categories.js
index 47da67b..36b2d9d 100644
--- a/app/assets/javascripts/discourse/app/controllers/reorder-categories.js
+++ b/app/assets/javascripts/discourse/app/controllers/reorder-categories.js
@@ -60,56 +60,97 @@ export default Controller.extend(ModalFunctionality, Evented, {
     this.notifyPropertyChange("categoriesBuffered");
   },
 
-  move(category, direction) {
-    let otherCategory;
+  countDescendants(category) {
+    return category.get("subcategories")
+      ? category
+          .get("subcategories")
+          .reduce(
+            (count, subcategory) => count + this.countDescendants(subcategory),
+            category.get("subcategories").length
+          )
+      : 0;
+  },
 
-    if (direction === -1) {
-      // First category above current one
-      const categoriesOrderedDesc = this.categoriesOrdered.reverse();
-      otherCategory = categoriesOrderedDesc.find(
-        (c) =>
-          category.get("parent_category_id") === c.get("parent_category_id") &&
-          c.get("position") < category.get("position")
-      );
-    } else if (direction === 1) {
-      // First category under current one
-      otherCategory = this.categoriesOrdered.find(
-        (c) =>
-          category.get("parent_category_id") === c.get("parent_category_id") &&
-          c.get("position") > category.get("position")
-      );
+  move(category, direction) {
+    let targetPosition = category.get("position") + direction;
+
+    // Adjust target position for sub-categories
+    if (direction > 0) {
+      // Moving down (position gets larger)
+      if (category.get("isParent")) {
+        // This category has subcategories, adjust targetPosition to account for them
+        let offset = this.countDescendants(category);
+        if (direction <= offset) {
+          // Only apply offset if target position is occupied by a subcategory
+          // Seems weird but fixes a UX quirk
+          targetPosition += offset;
+        }
+      }
     } else {
-      // Find category occupying target position
-      otherCategory = this.categoriesOrdered.find(
-        (c) => c.get("position") === category.get("position") + direction
+      // 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")) {
+        // 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");
+      }
     }
 
-    if (otherCategory) {
-      // Try to swap positions of the two categories
-      if (category.get("id") !== otherCategory.get("id")) {
-        const currentPosition = category.get("position");
-        category.set("position", otherCategory.get("position"));
-        otherCategory.set("position", currentPosition);
-      }
-    } else if (direction < 0) {
-      category.set("position", -1);
-    } else if (direction > 0) {
-      category.set("position", this.categoriesOrdered.length);
+    // Adjust target position for range bounds
+    if (targetPosition >= this.categoriesOrdered.length) {
+      // Set to max
+      targetPosition = this.categoriesOrdered.length - 1;
+    } else if (targetPosition < 0) {
+      // Set to min
+      targetPosition = 0;
     }
 
+    // Update other categories between current and target position
+    this.categoriesOrdered.map((c) => {
+      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);
+        }
+      } 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);
+        }
+      }
+    });
+
+    // Update this category's position to target position
+    category.set("position", targetPosition);
+
     this.reorder();
   },
 
   actions: {
     change(category, event) {
-      let newPosition = parseInt(event.target.value, 10);
-      newPosition = Math.min(
-        Math.max(newPosition, 0),
-        this.categoriesOrdered.length - 1
-      );
-
-      this.move(category, newPosition - category.get("position"));
+      let newPosition = parseFloat(event.target.value);
+      newPosition =
+        newPosition < category.get("position")
+          ? Math.ceil(newPosition)
+          : Math.floor(newPosition);
+      const direction = newPosition - category.get("position");
+      this.move(category, direction);
     },
 
     moveUp(category) {
diff --git a/app/assets/javascripts/discourse/tests/unit/controllers/reorder-categories-test.js b/app/assets/javascripts/discourse/tests/unit/controllers/reorder-categories-test.js
index 2feb422..821b2cb 100644
--- a/app/assets/javascripts/discourse/tests/unit/controllers/reorder-categories-test.js
+++ b/app/assets/javascripts/discourse/tests/unit/controllers/reorder-categories-test.js
@@ -11,17 +11,14 @@ discourseModule("Unit | Controller | reorder-categories", function () {
       categories.push(store.createRecord("category", { id: i, position: 0 }));
     }
 
-    const reorderCategoriesController = this.getController(
-      "reorder-categories",
-      { site: { categories } }
-    );
-    reorderCategoriesController.reorder();
+    const controller = this.getController("reorder-categories", {
+      site: { categories },
+    });
+    controller.reorder();
 
-    reorderCategoriesController
-      .get("categoriesOrdered")
-      .forEach((category, index) => {
-        assert.equal(category.get("position"), index);
-      });
+    controller.get("categoriesOrdered").forEach((category, index) => {
+      assert.equal(category.get("position"), index);
+    });
   });
 
   test("reorder places subcategories after their parent categories, while maintaining the relative order", function (assert) {
@@ -51,14 +48,13 @@ discourseModule("Unit | Controller | reorder-categories", function () {
     });
 
     const expectedOrderSlugs = ["parent", "child2", "child1", "other"];
-    const reorderCategoriesController = this.getController(
-      "reorder-categories",
-      { site: { categories: [child2, parent, other, child1] } }
-    );
-    reorderCategoriesController.reorder();
+    const controller = this.getController("reorder-categories", {
+      site: { categories: [child2, parent, other, child1] },
+    });
+    controller.reorder();
 
     assert.deepEqual(
-      reorderCategoriesController.get("categoriesOrdered").mapBy("slug"),
+      controller.get("categoriesOrdered").mapBy("slug"),
       expectedOrderSlugs
     );
   });
@@ -84,21 +80,18 @@ discourseModule("Unit | Controller | reorder-categories", function () {
       slug: "test",
     });
 
-    const reorderCategoriesController = this.getController(
-      "reorder-categories",
-      { site: { categories: [elem1, elem2, elem3] } }
-    );
+    const controller = this.getController("reorder-categories", {
+      site: { categories: [elem1, elem2, elem3] },
+    });
 
-    reorderCategoriesController.actions.change.call(
-      reorderCategoriesController,
-      elem1,
-      { target: { value: "2" } }
-    );
+    // Move category 'foo' from position 0 to position 2

[... diff too long, it was truncated ...]

GitHub sha: 7ba35e0d71a41a47be5efead91f413f650343b4b

This commit appears in #13013 which was approved by udan11. It was merged by udan11.