UX: Improvements for reordering categories (PR #13013)

This PR includes two UX changes for the reorder categories modal.

The first (and bigger) change ensures that all categories retain their relative order, except for the one being moved. The second allows decimal inputs in the position field for more precise ordering.

See commit messages for more details. Happy to address any questions or concerns.

GitHub

@udan11 can you review this one? I know you recently worked on sort order.

I don’t think the failing test has anything to do with this PR.

Failed Test rspec ./spec/components/topic_view_spec.rb:62 # TopicView custom filters allows to register custom filters

I would simplify the base case and simply check if subcategories is defined:

if (!category.subcategories) {
  return 0;
}

Even if it exists and is blank, when the function reaches such a category then it will not be called anymore and simply return category.subcategories.length which is 0.

It would be nice to see if category.subcategories is always set (never undefined or null). In which case, you could get rid of the base case completely.

The code looks mostly good. Most of the comments are nitpicks. By the way, explicit get is not needed anymore (since Ember Octane, I believe). For example, you can simply use category.isParent instead of category.get("isParent").

To be honest, I liked the old behavior more because you could easily swap two categories by typing the position OR maintaining the relative order by pressing the arrow buttons.

I prefer forEach over map here because we simply want to iterate without creating a new array. If you want to get fancy, you could use reduce:

category.subcategories.reduce((count, subcategory) => count + this.countDescendants(subcategory), category.subcategories.length)

This way, your method becomes a simple oneliner:

  countDescendants(category) {
    return category.subcategories ? category.subcategories.reduce((count, subcategory) => count + this.countDescendants(subcategory), category.subcategories.length) : 0;
  },

There is no need to compute direction twice just to adjust the position.

      if (newPosition < category.get("position")) {

Eventually, you could write the whole block such as:

newPosition = newPosition < category.get("position") ? Math.ceil(newPosition) : Math.floor(newPosition);

Thank you.

It would be nice to see if category.subcategories is always set (never undefined or null). In which case, you could get rid of the base case completely.

I tried removing the check for category.subcategories being set and got the following error:

Uncaught TypeError: category.get(...) is undefined

It seems like it is sometimes undefined.

This is a great suggestion. Thank you!

This is much better. Thanks!

The code looks mostly good. Most of the comments are nitpicks. Thanks for the review @udan11. I appreciate you’re time and feedback. I have finished making the changes you suggested and will push a new commit momentarily.

By the way, explicit get is not needed anymore (since Ember Octane, I believe). For example, you can simply use category.isParent instead of category.get("isParent").

Thanks! I tried to remove this and ended up with the following error.

Uncaught Error: Assertion Failed: You attempted to access the `subcategories` property (of <(unknown):ember1195>).
Since Ember 3.1, this is usually fine as you no longer need to use `.get()`
to access computed properties. However, in this case, the object in question
is a special kind of Ember object (a proxy). Therefore, it is still necessary
to use `.get('subcategories')` in this case.

To be honest, I liked the old behavior more because you could easily swap two categories by typing the position OR maintaining the relative order by pressing the arrow buttons.

That’s fair. It does add an extra step to swap two categories. However, it also saves potentially dozens of clicks if swapping is not what you were trying to do and you want to move a category from the bottom of the page near the top.

If you truly disagree that this is better UX, I am happy to start a topic on Meta about it to get more input.

@udan11 do you feel this is ready for a merge now?

Looks good to me, thanks! :+1: