Queue Tools: Grouping comments by submission

eritbh commented 7 years ago

Ref: #799

Starting this now because it's a thing that works, though there's a lot that could change with this depending on where we want to take it.

To discuss:

Keep option in settings, or put it on the page?
Style the groups? If not, we should ditch the wrappers.
If the queue is sorted by age descending (new at the top), the submission will always be at the bottom of its own list. Should we pull it up to the top and then have the comments within keep their existing order?
Currently the groups are sorted according to the thing that comes first in the group. Is that okay?
If a link isn't reported but several of its comments are, should the link preview be generated and put in for context?

TODO:

[ ] NER
[ ] Styles, if we're doing that
[ ] Fix things on dynamic sort

Other things that would be cool:

Collapse/expand group display
Select all in group (easily perform bulk actions on the group)

creesch commented 7 years ago

Just as a note to everyone don't merge this yet. I want to release 3.6 first before we merge this.

creesch commented 7 years ago

I still need to check it out, but my thoughts so far on your questions.

Keep option in settings, or put it on the page?

Hrm, settings for permanent and possibly a button to do it on the fly?

Style the groups? If not, we should ditch the wrappers.

Probably, I'll need to check out how it looks now.

If the queue is sorted by age descending (new at the top), the submission will always be at the bottom of its own list. Should we pull it up to the top and then have the comments within keep their existing order?

That might be a good idea.

Currently the groups are sorted according to the thing that comes first in the group. Is that okay?

Not sure I understand this question? You mean that if you have this list

comment 1, thread 1
comment 2, thread 2
comment 3 thread 1

That it will first show the group for thread 1? I think that is okay, I'll think it over for a bit and also play with it as well.

If a link isn't reported but several of its comments are, should the link preview be generated and put in for context?

Hrm... Not sure if it is really needed but it might provide some more context and clearly defines the groups.

eritbh commented 7 years ago

Not sure I understand this question?

Yeah, so basically if the original order is what you have there, it will come out looking like this:

comment 1, thread 1
comment 3, thread 1
comment 2, thread 2

Basically, once it finds a thread it hasn't seen yet, it pulls other comments from that thread up below it - as opposed to pulling them down or doing something stupid like averaging the positions.

eritbh commented 7 years ago

oh right also we have to handle NER for this at some point, fun

creesch commented 7 years ago

Yup... NER is a thing ;)

Should be fairly straightforward though, you can probably make it just append to the already existing groups. NER seems tricky at first but once you get the hang of it you can make it work rather easily.

Probably adjust the current function to add a class "tb-group-processed" once it has processed the ".thing" and at the beginning of the function do a if (!$thing.hasClass('tb-group-processed')) {}

eritbh commented 7 years ago

Yeah that sounds doable. Cool.

creesch commented 7 years ago

One more thing currently not working is sorting, when you resort the grouping gets undone.

A possible solution would be a "reset" function that undoes the wrappers and then call the group function again after sorting is done.

creesch commented 7 years ago

I also think that instead of adding a horizontal line as physical element border work better. Something like

.tb-comment-group {
    border: solid 3px #d2d2d2;
    margin-bottom: 5px;
    margin-right: 310px;
    padding-top: 5px;
}

Note that the right margin probably should be determined on the fly just as with .menuarea.modtools because of subreddit css and sidebars.

eritbh commented 7 years ago

Er, dynamic sorting is a thing? I thought that involved a page reload anyway... probably remembering the module wrong. Can't test anything rn but that sounds like something I could do.

And yes, the plan was to move any actual styles to CSS instead of using

s in the future. I just added that as part of the mockup. (Pretty sure it'd be padding-bottom, not padding-top, though.)

Honestly I think we'd be fine if we didn't even bother trying to figure out what margin-right should be; if the sidebar is floated, the width of the separator should adjust appropriately just like the links themselves cut off, and if not, then it will just follow the same flow as the links/comments themselves. (I think? It's actually been a while since I worked with a floating sidebar design like that in-depth, I reserve the right to be completely wrong :V )

creesch commented 7 years ago

Er, dynamic sorting is a thing?

Yessir.

Pretty sure it'd be padding-bottom, not padding-top, though.

Na padding top in this case because the first element is otherwise clinging to the top border. Just aesthetic kind of thing :) There are probably a few other methods we could try and style it, but I figured that since it is group I would do a border around the entire thing.

Honestly I think we'd be fine if we didn't even bother trying to figure out what margin-right should be; if the sidebar is floated, the width of the separator should adjust appropriately just like the links themselves cut off, and if not, then it will just follow the same flow as the links/comments themselves.

Nah that looks all ugly and you have to account for all sorts of things, besides the code to determine what the margin needs to be is already in there ;)

eritbh commented 7 years ago

otherwise clinging to the top border.

Doesn't the margin-bottom handle that though...? http://codepen.io/Geo1088/pen/LxMoBb

that looks all ugly and you have to account for all sorts of things

Fair enough, but will it be easy to only apply it to the area where the sidebar restricts the content width? Otherwise the wrappers wouldn't let their content flow underneath the sidebar on subs that allow that to happen.

I'll have to try it out with some other styles and see how things look.

creesch commented 7 years ago

The clinging part is the inner side of the wrapper. I should have made a screenshot I suppose.

Sidebar always restricts content width because it otherwise will flow under the sidebar obscuring the right border.

eritbh commented 7 years ago

Sidebar always restricts content width because it otherwise will flow under the sidebar obscuring the right border.

I'm not even sure we're talking about the same thing? Content is supposed to flow underneath the sidebar unless the sub sets it to not.

shitty screenshot

creesch commented 7 years ago

I shall screenshot it :)

eritbh commented 7 years ago

This branch will probably go stale now that I won't be able to actively work on toolbox for a while. If you guys want to add to this, you should have push access to this branch, but it'll require you cloning my copy of the repo and making changes from there.

agentlame commented 7 years ago

Where are we on this, now that 3.6 is out?

creesch commented 7 years ago

I'll pick up work on this I guess, it is still in alpha stage in some regards.

creesch commented 7 years ago

Okay, I am merging this in so we can work on it now that 3.6 is out and I hotfixes have been done.

toolbox-team / reddit-moderator-toolbox-legacy