Feat: Enable sorting discussions by original comment #422
Conversation
jakubbortlik
force-pushed
the
408-make-sorting-discussions-customizable
branch
from
c1247ac to
5f44961
Compare
|
This sorting seems backward to me, when I sort I'd expect the newest comments to appear on top, here they appear on the bottom |
| else | ||
| state.settings.discussion_tree.sort_by = "original_comment" | ||
| end | ||
| u.notify("Sort discussion tree by '" .. state.settings.discussion_tree.sort_by .. "'", vim.log.levels.INFO) |
There was a problem hiding this comment.
Rather than having this be a notification, I think this information ought to live in the winbar! For instance "Ascending by discussion" or something like that, to the left of the Live Mode/Draft Mode text.
There was a problem hiding this comment.
There were several reasons I made this just a notification.
- The sorting is mostly obvious from the tree itself
- The user will typically know what their default setting is and the notification is just an extra confirmation that the mapping worked as expected, but it's OK if the information disappears again
- When the discussion split is placed to the left or right of the reviewer, the winbar text can easily overflow even without this extra piece of information.
If anything, I think this could be a single character, something like vand ^, maybe with a different highlight color to make it more visible.
What do you think?
There was a problem hiding this comment.
I'm thinking just the arrow would work. I'd like to show this state to the user since it's persisted and affects the view of the discussion tree.
It's very possible we'll want to add filtering/searching by text to the winbar, so keeping the sort direction as short as possible is probably best to make room for that.
There was a problem hiding this comment.
Actually, I'm addressing the winbar width in #435. Maybe have a look at that PR and let me know if you in general agree with the idea. Then we can find a "shrinkable" way for showing the sort order.
There was a problem hiding this comment.
What about these variants of the description that would be selected based on available space:
↑ by reply->↑ reply->↑↓ by thread->↓ thread->↓
I think Unicode charactersU+2191andU+2193should not be a problem for any user.
There was a problem hiding this comment.
This looks good to me! Short and descriptive.
There was a problem hiding this comment.
I've removed the notification and made a couple of small improvements.
The discussion tree takes some time to rebuild and for a short time there is a discrepancy between the indicated sort method and the actual sorting. This will be less of a problem if harrisoncramer#432 is merged so the user will see that the discussion tree is being refreshed.
jakubbortlik
force-pushed
the
408-make-sorting-discussions-customizable
branch
from
57c5530 to
4a456bf
Compare
* Feat: Enable sorting discussions by original comment (#422) * Feat: Improve popup UX (#426) * Feat: Automatically update MR summary details (#427) * Feat: Show update progress in winbar (#432) * Feat: Abbreviate winbar (#439) * Fix: Note Creation Bug (#441) * Fix: Checking whether comment can be created (#434) * Fix: Syntax in discussion tree (#433) * fix: improve indication of resolved threads and drafts (#442) * Docs: Various minor improvements (#445) --------- Co-authored-by: Jakub F. Bortlík <jakub.bortlik@proton.me>
2 participants
Resolves #408.
This PR modifies the Go server in order to add a different sorting order for discussions. Users can now choose between the original sorting order by
latest_reply, in which the the recently active threads are on top of the tree and byoriginal_comment, in which the threads are sorted chronologically from oldest on to to newest (just like on Gitlab online).There're theoretically two more sorting orders, in which the order is reversed for the two existing options, by in my view they are not really practical so I haven't included them, since that would require a more complicated solution (the config
settings.discussion_tree.sort_bywould have to be a table, rather than a simple string. Of course, if you @harrisoncramer think that you'd actually want it to be more versatile and future-proof, I can change the PR so that thesort_byconfig option is a table.