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.

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

server assisted comment collapsing #397

Merged
merged 2 commits into from
Jan 12, 2024

Conversation

asdfzdfj
Copy link
Contributor

@asdfzdfj asdfzdfj commented Jan 4, 2024

NOTE: this is very much a WIP, things may change wildly, expect frequent rebase/force push a lot of things has settled down, but still definitely needs some real world tests and feedback, not a UI/UX person

add ability to collapse a comment's replies, hiding them away and minimized the parent comment

somewhat based on klin's work (https://codeberg.org/Kbin/kbin-core/pulls/167) but with a few of my own differences:

  • use depth information from twig side, rather than sleuthing from css comment-level-- class.
    this means it can collapse comments with >10 depth more accurately
  • if collapsing a comment's replies and then collapse/expand this comment's parent, the nested reply collapse will be retained (klin's work will always expand all replies although it was collapsed by a different, nested parent)
  • visual changes only, no dom node insertion/deletion used, nor planned to

the collapse button is placed at the top right corner of the comments, after the vote button

image
image

since collapsing comments will also minimize the parent, it can be attached to comment with no replies for minimizing effect.
the collapse button will use slightly different icons to signify this

it all look something like this

image


previous concerns

latest version (both commit): icon button at the top right of the comment
image
image

first commit version: text button at the bottom right of the comment
image
image

the top corner one seems to be more preferred, if this is settled then I'll squashed both version/commits together so I could focus on refining the patch


there are quite a lot of things that I'm not sure how to go with it, there's probably more but this is what I could think of at the moment:

  1. should the collapsing process strictly relies on visual depth, real comment depth, or some mix of both

    • klin's work and kbin impl only uses existing depth from comment-level--{1..10} css class

    • my impl used depth from twig side and without cap, which means it could collapse deeper (>10 depth) comment more accurately but could lead to a rather unintuitive experience since you could collapse comments that appeared to be on the same depth (which happens quite often when you're reading Foone posts, see this in action)

      image

    • but then I have decided to only enable collapse button on top level comments in classic view, which aligns more with the visual depth

  2. should collapsing action only fold the replies or should it 'minimize' the parent? (the one you clicked collapse)
    klin's work and my old impl does this, kbin and this impl doesn't. it does now.

  3. where should the collapse button be?

    • (currently) in menu bar, like the post replies' expand/collapse button

      image
      image

    • left corner, like kbin

      image
      image

    • right corner, my old impl does this. now this patch does too
      perhaps it make a bit more sense with minimizing parents, as the expand button is there when minimized, and so the collapse/expand wouldn't move too far when toggling them

      image
      image

my old impl attempt for reference: https://github.com/asdfzdfj/mbin/tree/monkey/server-assisted-comment-collapse-old

@asdfzdfj asdfzdfj added the needs feedback Requires a greater consensus to make an informed decision label Jan 4, 2024
@e-five256
Copy link
Member

e-five256 commented Jan 4, 2024

  1. I'm not quite getting what you mean, or rather what it would be like if it worked differently. If I had to guess, is it because of spots like this where:
    image
    the collapse button suddenly disappears, but then reappears?

  2. likewise I'm not quite getting what you mean here. if I go to kbin and collapse a top comment, it appears to work exactly the same. the only difference I do notice is that if I collapse a child reply, then a top level, then expand the top level, all the children are expanded in kbin whereas for your change the collapse state is kept

  3. (tbh I kind of prefer how reddit works with top left 🔽, collapse the one you click collapse on and anything under it, not just the things under it while leaving the top level uncollapsed. but if we're going with this way, I definitely prefer the word, I would just move it to the right of boost so reply / boost are always consistent as when collapse isn't there it shifts the ux in a surprising way I might not realize while just trying to click boost down a chain)

(edit: specifically because it allows collapsing top level comments. with the current impl you have to live with seeing top level replies no matter what, and in my experience on reddit a lot of times those are just hate speech and intolerance, which I guess is a different reason to collapse now that I think about it most people likely did it due to being done reading a section or cleaning up the interface, I only did it when it was stuff like I said)

@asdfzdfj
Copy link
Contributor Author

asdfzdfj commented Jan 5, 2024

I should have said this from the start, but my plan for this patch is that I'll be remaking the old change what what I learned from the previous attempt, and here's my old impl attempt for reference: https://github.com/asdfzdfj/mbin/tree/monkey/server-assisted-comment-collapse-old

  1. I'm not quite getting what you mean, or rather what it would be like if it worked differently. If I had to guess, is it because of spots like this where:
    image
    the collapse button suddenly disappears, but then reappears?

that's part of it, but there's also the fact that the collapse button exists there at all

I found kbin.social counterpart of this posts chain, notice that the collapse button only appears when the replies that would be collapsed has a clear visual indentation on it, and no more collapse button on comments of the same depth although the replies chain actually goes deeper

my concerns is that for the usual users deep down the replies chain, the presence of the collapsing button can feel rather erratic, and "reading" what would be collapsed can be harder since there's no indentation to read from, unless they're aware that it tries to work on real depth where things might start to make more sense, but they should be able to understand the behavior intuitively and not needing to know what's going on under the hood

  1. likewise I'm not quite getting what you mean here. if I go to kbin and collapse a top comment, it appears to work exactly the same. the only difference I do notice is that if I collapse a child reply, then a top level, then expand the top level, all the children are expanded in kbin whereas for your change the collapse state is kept

  2. (tbh I kind of prefer how reddit works with top left 🔽, collapse the one you click collapse on and anything under it, not just the things under it while leaving the top level uncollapsed. but if we're going with this way, I definitely prefer the word, I would just move it to the right of boost so reply / boost are always consistent as when collapse isn't there it shifts the ux in a surprising way I might not realize while just trying to click boost down a chain)

(edit: specifically because it allows collapsing top level comments. with the current impl you have to live with seeing top level replies no matter what, and in my experience on reddit a lot of times those are just hate speech and intolerance, which I guess is a different reason to collapse now that I think about it most people likely did it due to being done reading a section or cleaning up the interface, I only did it when it was stuff like I said)

ok, so here's the old implementation attempt in a nutshell:

image

  • collapsing comments would hides its replies and also collapse itself, I didn't included it here yet because I saw that kbin's impl doesn't do this, and the styling used to achieve that back then is a little bit messy. so I'm not sure if minimizing parents to is worth it but it's possible to still do it if desired
  • expand/collapse button at the bottom right corner, it was put there because putting expand button on collapsed comments there reduces the collapsed comments size better than just hiding the content and vote buttons (can't just hide the menu since the expand button is also in there)
  • the collapse button is also shown for comments with no replies since it's also work for minimizing the comment, but that arguably makes reading collapsed target even harder

@asdfzdfj asdfzdfj force-pushed the new/server-assisted-comment-collapse branch 4 times, most recently from 71e1b98 to 9e755f4 Compare January 6, 2024 11:19
@asdfzdfj
Copy link
Contributor Author

asdfzdfj commented Jan 6, 2024

well I've decided to added back the minimizing parent and putting the collapse button on the lower right corner, those are just too good to discard (personally). give it a shot.

also this means the collapse button could be enabled for any comment for the minimizing effect on comment without reply,
but with that I'm also think if the button label for should be changed to reduce visual clutter and maybe differentiate that clicking collapse would also hide its replies or not.

image

@e-five256
Copy link
Member

e-five256 commented Jan 6, 2024

I do like it better hiding the one you click on, and as you say would like to see it on all posts even if they don't have replies (and yes maybe the words "hide/show" are better descriptive in that case)

The only thing I'm uncertain of is the position. It'd be nice if it could not shift the position of elements, like for example, just taking the thread with the most comments https://old.lemmy.world/post/3051842 if I click hide on the top post there the show button is in the exact same place on my mouse, so I can just spam click the hide/show button. That would kind of be my ideal, but I understand if that's difficult to achieve

edit: I also didn't quite notice this is mainly JS driven, I guess that's a must because on microblogs it loads replies via ajax requests? with slow to load JS it is a bit weird but I guess it is what it is

@e-five256
Copy link
Member

I don't want this all be solely my feedback though, maybe we can get a second opinion. Honestly I think any of the options are improvements so I'd be fine with going ahead with any of them as well, @nobodyatroot any thoughts?

@asdfzdfj asdfzdfj force-pushed the new/server-assisted-comment-collapse branch 2 times, most recently from f8ea0f3 to f673bf9 Compare January 7, 2024 08:43
@asdfzdfj
Copy link
Contributor Author

asdfzdfj commented Jan 7, 2024

I do like it better hiding the one you click on, and as you say would like to see it on all posts even if they don't have replies (and yes maybe the words "hide/show" are better descriptive in that case)

I have added back the button for all comments, and also trying to use show/hide for button label in that case

The only thing I'm uncertain of is the position. It'd be nice if it could not shift the position of elements, like for example, just taking the thread with the most comments https://old.lemmy.world/post/3051842 if I click hide on the top post there the show button is in the exact same place on my mouse, so I can just spam click the hide/show button. That would kind of be my ideal, but I understand if that's difficult to achieve

I think then the collapse button would need to be moved to somewhere near the top of the comment element for this to work, and where I put it at the moment (bottom right) is arguably the worst place to put it this property is desired

maybe the collapse button could be put around the same place as the vote buttons but then I don't know how it should be designed

edit: I also didn't quite notice this is mainly JS driven, I guess that's a must because on microblogs it loads replies via ajax requests? with slow to load JS it is a bit weird but I guess it is what it is

I don't think microblog having to load expanded replies from ajax has anything to do with this, and it's actually possible to do this without using js (Postmill has done it), but in order to do that the comment rendering would need to be restructured so that the replies are nested in a section/div:

<post>
    <div>main comment body</div>
    <div class="replies">
        <!-- nested replies ... -->
    </div>
</post>

(@BentiGorlich has actually mentioned this restructuring before at least once, but for the reason that the act of hiding replies would be as simple as just hiding the div.replies element)

but this would be quite a major undertaking, and there're also other js stuff that can insert comments live, those would need to be updated to handle this new structure too.

and after doing the sensitive image toggle stuff I have to say that I'm not particularly motivated to do frontend stuff right now,
also this patch started its life as an attempt to update klin's work but based on a newer codebase than when the PR was filed (with a bit of my own spins), long before knowing that it could be done without js.

@e-five256
Copy link
Member

Looking at it on your instance, it looks good to me

@asdfzdfj asdfzdfj force-pushed the new/server-assisted-comment-collapse branch from f673bf9 to 85727d2 Compare January 8, 2024 05:25
@asdfzdfj
Copy link
Contributor Author

asdfzdfj commented Jan 8, 2024

The only thing I'm uncertain of is the position. It'd be nice if it could not shift the position of elements, like for example, just taking the thread with the most comments https://old.lemmy.world/post/3051842 if I click hide on the top post there the show button is in the exact same place on my mouse, so I can just spam click the hide/show button. That would kind of be my ideal, but I understand if that's difficult to achieve

in any case I added an experiment patch that moves the collapse/expand button to the top right corner of the comment, after the votes button. try switching between the base version and this alternate button version and see which one is more practical

this way the button and the screen stays roughly at the same place when collapsing/expanding comment,
though this does come with a tradeoff where the button has to be icon in order to guarantee layout consistency.

image

@asdfzdfj asdfzdfj force-pushed the new/server-assisted-comment-collapse branch 4 times, most recently from 4fc9ce7 to 18b535e Compare January 9, 2024 11:24
@BentiGorlich
Copy link
Member

I think the button style got removed when replying to a comment:
grafik

@asdfzdfj
Copy link
Contributor Author

asdfzdfj commented Jan 9, 2024

I also saw this yesterday, added a fix. try update the patch, it should be fine now

@BentiGorlich
Copy link
Member

I noticed that deleted comments do not have the collapse button:
grafik

@asdfzdfj
Copy link
Contributor Author

noted, will see what I could do.

in any case, it looks like the top right icon collapse button is preferred, it that's settled then I'll go ahead and squash both commits so I could go ahead and focus more on refining the patch

@asdfzdfj asdfzdfj force-pushed the new/server-assisted-comment-collapse branch 2 times, most recently from 4d89776 to 4686127 Compare January 10, 2024 07:17
@BentiGorlich
Copy link
Member

noted, will see what I could do.

👍

in any case, it looks like the top right icon collapse button is preferred, it that's settled then I'll go ahead and squash both commits so I could go ahead and focus more on refining the patch

absolutely. Btw. I looked at how kbin is doing it, and I don't like it :D

@asdfzdfj asdfzdfj force-pushed the new/server-assisted-comment-collapse branch 2 times, most recently from 45c16cc to aabd8b8 Compare January 10, 2024 09:48
@asdfzdfj asdfzdfj changed the title WIP: server assisted comment collapsing server assisted comment collapsing Jan 10, 2024
@asdfzdfj asdfzdfj marked this pull request as ready for review January 10, 2024 09:58
@asdfzdfj
Copy link
Contributor Author

pushed the update that should add back the collapsed button on deleted comments; squashed both commits together, keeping the top right icon button; and for the most part it seems to work good enough that I think it should be ready for review/merge now.

@asdfzdfj asdfzdfj added enhancement New feature or request frontend Visual issues, improvements, bugs or other aspects relating mostly to the front end labels Jan 10, 2024
Copy link
Member

@BentiGorlich BentiGorlich left a comment

Choose a reason for hiding this comment

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

The twig components lgtm. I don't really know how the js controller stuff works, but its working as intended, so I'll approve

add ability to collapse a comment's replies, hiding them away and
minimize the parent comment

somewhat based on klin's work (https://codeberg.org/Kbin/kbin-core/pulls/167)
but with a few of my own differences:
- use depth information from twig side, rather than sleuthing from css
  `comment-level--` class.
  this means it can collapse comments with >10 depth more accurately
- when collapsing a comment's replies and then collapse/expand its
  comment's parent, the nested reply collapse will be retained
  (klin's work will always expand all replies although it was collapsed
  by a different, nested parent)
- visual changes only, no dom node insertion/deletion used,
  nor planned to

the collapse button is placed at the top right corner of the comments,
after the vote button

since collapsing comments will also minimize the parent, it can be
attached to comment with no replies for minimizing effect.
the collapse button will use slightly different icons to signify this
@asdfzdfj asdfzdfj force-pushed the new/server-assisted-comment-collapse branch from 063cc95 to 2a8ceb2 Compare January 12, 2024 04:24
@asdfzdfj asdfzdfj merged commit 7de7c45 into main Jan 12, 2024
7 checks passed
@asdfzdfj asdfzdfj deleted the new/server-assisted-comment-collapse branch January 12, 2024 04:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request frontend Visual issues, improvements, bugs or other aspects relating mostly to the front end needs feedback Requires a greater consensus to make an informed decision
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants