Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix collection sorting for grouped collections #22392

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Fix collection sorting for grouped collections #22392

wants to merge 16 commits into from

Conversation

hanneskuettner
Copy link
Member

@hanneskuettner hanneskuettner commented May 6, 2024 •
edited
Loading

Scope

When displaying collections (in selects, lists, system-collection and system-collections) they were only sorted by meta.sort (and the collection name for ties). When grouping selections the sort index starts at 1 again, so the collections appeared out of order with how you would expect it based on the data-model manual sorting.

What's changed:

  • Introduced flattenGroupedCollections (not sure about the name, any suggestions?) to recursively flatten a list of collections that might be (deeply) grouped with the meta.group prop.
  • After talking with @paescuj I moved the sorting to a single source of truth, the collections store
    • Introduced a new sortedCollections getter in addition to the original collections state, that is now used instead of collections whenever a sorted list of collections is needed. As such the collections is not modified and can be used in a backward compatible manner (also it is assigned to outside of the store, introducing the need for either a replaceCollections or the sorted getter)
  • Added more getters in the collection store to ease repetitive filtering, and to use as intermediate lists in the store itself.

Potential Risks / Drawbacks

None

Review Notes / Questions

  • Did I miss any access to .collections that would result in inconsistent sorting behavior?
  • Is the sorting actually less confusing? Sometimes I still struggle with finding the collection easily.

Fixes #21658

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented May 6, 2024 •
edited
Loading

🦋 Changeset detected

Latest commit: d3a1bf1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@directus/app Patch
@directus/api Patch
directus Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@hanneskuettner hanneskuettner changed the title Fix collection for grouped collections Fix collection sorting for grouped collections May 6, 2024
@paescuj paescuj self-assigned this May 6, 2024
@paescuj paescuj marked this pull request as draft May 8, 2024 12:43
@hanneskuettner hanneskuettner mentioned this pull request Jun 24, 2024
Closed
@hanneskuettner hanneskuettner marked this pull request as ready for review July 23, 2024 18:42
@rijkvanzanten rijkvanzanten removed the 🔥 label Sep 2, 2024
@rijkvanzanten
Copy link
Member

@hanneskuettner It seems like non-table Groups are also present in the list when selecting a relational field, so somewhere we're not filtering those out as we should

Is the sorting actually less confusing? Sometimes I still struggle with finding the collection easily.

Good question.. I'm a little conflicted. Is it better to show them in the dropdown as actual groups you have to click through to open them? Alternatively, maybe we want to prefix nested ones with a space or two and an icon? F.e.

collection_a
group_a
  → collection_b
  → collection_c
  → nested_group
    → collection_d

although that'll quickly grow wider than the dropdown box 🤔

@AlexGaillard
Copy link
Member

AlexGaillard commented Sep 13, 2024 •
edited
Loading

Is it better to show them in the dropdown as actual groups you have to click through to open them

I think this makes the most sense to me as it keeps it much neater. Something like this.

image

You do run the risk of getting lost in the nest however. Though you could maybe have a little breadcrumb trail along the top?

maybe we want to prefix nested ones with a space or two and an icon

You could also perhaps split the difference where you can dropdown to expand for one level, and then going one level deeper sort of slides everything along, so you can only ever see two levels at a time. 🤔 If that makes sense. Might be a nightmare to dev though.

AlexGaillard
AlexGaillard approved these changes Sep 24, 2024
View reviewed changes
Copy link
Member

@AlexGaillard AlexGaillard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this LGTM.

We never addressed the idea of redesigning the UI: #22392 (comment)

However, I think that would likely balloon in scope so probably best to leave it as is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paescuj paescuj paescuj left review comments

@rijkvanzanten rijkvanzanten rijkvanzanten requested changes

@AlexGaillard AlexGaillard AlexGaillard approved these changes

Requested changes must be addressed to merge this pull request.

Assignees

@rijkvanzanten rijkvanzanten

Labels
Improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Order of related items is random, on 10.9.3
5 participants
@hanneskuettner @rijkvanzanten @AlexGaillard @paescuj @connorwinston