LB-1626: Sort Release Groups on Artist Page #2985
Conversation
Since we're sorting the release groups in the frontend, so sorting the release groups in the backend is not required.
anshg1214
changed the title
…erver into ansh/sort-rg-artist-page
There was a problem hiding this comment.
From what I can tell after deployment on test.LB, the sorting isn't working well yet.
I think we have made some assumptions a bit too fast, and maybe we are doing some operations in the wrong order.
I'll take this complex page as an example (lot of different types):
https://test.listenbrainz.org/artist/f58384a4-2ad2-4f24-89c5-c7b74ae1cce7/
If you look at the albums and compare to the MB page, there are only a handful of items there and they are not the albums I would expect to see (there are some demos, compilations, and about 20 albums missing)
I think the main issue is doing the groupBy operation after the sortBy. We should group first and apply the sort to each group.
Thanks to the react query devtools I can get the original server to try some things:
Sort by primary type and by first secondary type:
_.groupBy(releaseGroups, rg => rg.type + (rg.secondary_types?.[0] ? " + " + rg.secondary_types?.[0] : ""))
We end up with something close to what MB displays:
Applying the sortBy (as you coded it in this PR) for each of these groups gives me the expected result.
If we don't want to show all these different sections, we can probably show primary-type-only section followed by a group composed of that same primary type + all other secondary types.
For example based on the screenshot of groups above:
- Group 1: Album
- Group 2: Album+Live, Album+Compilation,Album+Demo,Album+Remix
- Group 3: Single
- Group 4: Single+Live (Single+Anything would go here too)
- Group 5: EP
- Group 6: EP+Live (EP+Anything would go here too)
- etc.
For this, something along those lines should work:
_.groupBy(releaseGroups, rg => rg.type + (rg.secondary_types?.length ? "+" : ""))
More likely than not, we probably want to select specific secondary types to show separate, and clump some other ones together.
For example, I think 1. Album, 2. Album+Live, and 3. (Album+Compilation, Album+Demo,Album+Remix) could be a good way to split it, i.e. keep live albums separate but clump the rest together.
Let me know if you want to have a chat about this in more details
|
@MonkeyDo I've made the respective changes to show all the primary types and secondary types similar to how MusicBrainz shows This PR is live on https://test.listenbrainz.org/ |
There was a problem hiding this comment.
It's looking a hundred times better after the last pass !
I agree with your decision to follow the MB way, as we quite clearly don't knnow enough between the two of us to account for all the weird ways items can be split or combined depending on type, nor have a clear idea of what our users would want to see.
At least with type + secondary type it is clear!
Final couple of changes and I think this is ready.
Co-authored-by: Monkey Do <[email protected]>
This PR fixes LB-1626.
But now since we're displaying more release group types, we need to fix LB-1536 on priority.