Sort By Set #358
Sort By Set #358
Conversation
✅ Deploy Preview for upset2 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
One point of discussion here is that I'd like to add tests for UpSet plot interactions such as aggregate, sort, etc; however, it doesn't seem like there is a good way to determine the order of the subsets on the plot, as their selectors are based on the position. Maybe adding an HTML id to each rendered row could be the solution to this. |
Addressed in ab1a972 |
packages/core/src/sort.ts
Outdated
| * @param sortBy - The set to sort by. | ||
| * @param vSetSortBy - The sort order for visible sets. | ||
| * @param visibleSets - The visible sets. | ||
| * @param sortByOrder - The sort order for the specified set. (optional) |
There was a problem hiding this comment.
Looks like this param no longer exists?
|
|
||
| // combine the two sorted row lists | ||
| const sortedOrder = sortedSetMembers.order.concat(sortedNonSetMembers.order); | ||
| const sortedValues = { ...sortedSetMembers.values, ...sortedNonSetMembers.values }; |
There was a problem hiding this comment.
Why use concat for one and spread/recombine for the other? If the goal is the same, same notation is more readable. Otherwise a comment explaining the implementation difference would be nice.
| @@ -60,7 +60,8 @@ const sortByAction = registry.register( | |||
| 'sort-by', | |||
| (state, { sort, sortByOrder }) => { | |||
| state.sortBy = sort; | |||
| state.sortByOrder = sortByOrder; | |||
| // should only be 'None' if sortBy is a Set | |||
| state.sortByOrder = sortByOrder || 'None'; | |||
There was a problem hiding this comment.
I think using null instead of 'none' here is slightly more type-safe and makes sense semantically.
There was a problem hiding this comment.
Or does state.sortByOrder need to be a string?
There was a problem hiding this comment.
You're right that state.sortByOrder is a string. Using 'None' to me makes sense as there is no order for sorting by set.
Does this PR close any open issues?
Closes #146
Give a longer description of what this PR addresses and why it's needed
This PR adds the ability to sort the UpSet plot by a set, aka, bringing a set to the top. See #113 for example context menu for sets.
The implemented behavior is based on the original Upset behavior here: http://vcg.github.io/upset/
This PR also removes any reference to the SortBy type, as this is only a string alias.
Provide pictures/videos of the behavior before and after these changes (optional)
BEFORE:
AFTER:
Have you added or updated relevant tests?
Have you added or updated relevant documentation?
Are there any additional TODOs before this PR is ready to go?
TODOs: