From f72ba754f1f5648303b72a1e6c97635a55c7f306 Mon Sep 17 00:00:00 2001 From: Jarek Radosz Date: Thu, 1 Feb 2024 13:13:38 +0100 Subject: [PATCH] DEV: Rework reorder-categories modal (#25475) --- .../components/modal/reorder-categories.hbs | 24 +- .../components/modal/reorder-categories.js | 162 +++++++------- .../components/reorder-categories-test.gjs | 171 +++++++++++++++ .../components/reorder-categories-test.js | 205 ------------------ .../common/base/reorder-categories.scss | 13 +- 5 files changed, 269 insertions(+), 306 deletions(-) create mode 100644 app/assets/javascripts/discourse/tests/integration/components/reorder-categories-test.gjs delete mode 100644 app/assets/javascripts/discourse/tests/unit/components/reorder-categories-test.js diff --git a/app/assets/javascripts/discourse/app/components/modal/reorder-categories.hbs b/app/assets/javascripts/discourse/app/components/modal/reorder-categories.hbs index fea2d492637..ba7e6b4bc97 100644 --- a/app/assets/javascripts/discourse/app/components/modal/reorder-categories.hbs +++ b/app/assets/javascripts/discourse/app/components/modal/reorder-categories.hbs @@ -1,6 +1,7 @@ <:body> @@ -10,11 +11,11 @@ {{i18n "categories.reorder.position"}} - {{#each this.categoriesOrdered as |category|}} - + {{#each this.sortedEntries as |entry|}} + -
- {{category-badge category allowUncategorized="true"}} +
+ {{category-badge entry.category allowUncategorized="true"}}
@@ -22,22 +23,22 @@
@@ -51,6 +52,7 @@ diff --git a/app/assets/javascripts/discourse/app/components/modal/reorder-categories.js b/app/assets/javascripts/discourse/app/components/modal/reorder-categories.js index 803a84da65b..8eaafeaafdb 100644 --- a/app/assets/javascripts/discourse/app/components/modal/reorder-categories.js +++ b/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 { diff --git a/app/assets/javascripts/discourse/tests/integration/components/reorder-categories-test.gjs b/app/assets/javascripts/discourse/tests/integration/components/reorder-categories-test.gjs new file mode 100644 index 00000000000..826564e437e --- /dev/null +++ b/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(); + + 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(); + + 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(); + + // 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(); + 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(); + 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"); + }); +}); diff --git a/app/assets/javascripts/discourse/tests/unit/components/reorder-categories-test.js b/app/assets/javascripts/discourse/tests/unit/components/reorder-categories-test.js deleted file mode 100644 index e0d604845c1..00000000000 --- a/app/assets/javascripts/discourse/tests/unit/components/reorder-categories-test.js +++ /dev/null @@ -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", - ]); - }); -}); diff --git a/app/assets/stylesheets/common/base/reorder-categories.scss b/app/assets/stylesheets/common/base/reorder-categories.scss index 7b51fcc23f2..01947fcb54d 100644 --- a/app/assets/stylesheets/common/base/reorder-categories.scss +++ b/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) {