Add collapsable threads #7488
Add collapsable threads #7488
Conversation
| for (let val of skeleton.replies) { | ||
| if (!isThreadPost(val)) continue | ||
| if (collapsedCIDs.has(val.post.cid) || set.has(val.post.cid)) { | ||
| val.replies?.forEach(r => { | ||
| isThreadPost(r) && set.add(r.post.cid) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
This traverses the tree of posts any time a post is collapsed, which may not be efficient enough in production
This comment was marked as outdated.
This comment was marked as outdated.
|
Might be easier to get this merged if you split the thread collapsing and actions rework into separate PRs |
|
That's fair, but I do like how it currently preserves positioning of the controls between states, as a bonus on top of the additional post density. Can appreciate others may not see the density as an advantage, happy to split if it becomes a point of debate |
vcarl
force-pushed
the
vcarl-collapsable-threads
branch
from
95922eb to
c777b9a
Compare
|
Opted to split up the controls changes from the collapse behavior, after using it on a phone and feeling like deeply nested replies got too squished with the horizontal space taken up by controls 
|
vcarl
changed the title
| @@ -255,6 +257,28 @@ export function PostThread({uri}: {uri: string | undefined}) { | |||
| randomCache, | |||
| ]) | |||
|
|
|||
| const [collapsedCIDs, setCollapsedCIDs] = React.useState(new Set()) | |||
There was a problem hiding this comment.
I think you want to use URIs rather than CIDs here. URIs are what we use as unique identifiers for posts
|
I'd love to have this as an option alongside the regular Threaded mode. Maybe this could be called Threaded (Compact)?
|
|
The failing checks are on files I didn't touch 👀 |
|
@surfdude29 I don't think the collapse mechanic needs to be under a new UI interaction mode. If you're referencing my previously proposed alternate controls experience, I want to table that suggestion for the purposes of this PR to keep it focused on the collapse behavior — I haven't yet opened a new PR for the UI change but I could if desired |
|
Rebasing should fix the CI |
|
@vcarl Sorry if I misunderstood, I was assuming this would be as an alternate option to the existing Threaded mode? Because otherwise this part:
would be a regression from the current Threaded mode (which would lead to a great deal of user confusion). Please correct me if I'm misunderstanding though. |
vcarl
force-pushed
the
vcarl-collapsable-threads
branch
from
6bfe125 to
6ce33cb
Compare
|
"Regression" is a technical term regarding the reintroduction of a previously fixed bug; your complaint is about a UX decision I've made, not a regression. This is a change to the rendering of replies to a selected post, which I believe to be correct on the hypothesis that people using "Threaded Mode" are seeking a social media experience akin to reddit. Through that lens, replies to a top-level post are more like "comments" than posts in their own right. As such, deemphasizing attachments in turn emphasizes the contents of the replies themselves in a way that I believe is more desired by those who use Threaded Mode — or, maybe I'm just speaking for myself, as this is my own preference. It is a change to behavior though so if there's agreement that it needs its own PR I can make that alteration. This PR was the end result of playing in the codebase for an afternoon so I can accept if some of the changes aren't meant to be. Here's a comparison of this PR: 
Vs production appearance: 
It's a pretty small change, and one I personally strongly prefer — I don't want some 5th-level reply taking up a third of my screen just because they shared a link. Links above the selected post, i.e. post ancestors, will still show link previews. |
Your line of reasoning does have a lot of merit. To explain my thinking further, I'm of the view that link cards and external media embeds are an important part of the user experience, whether they're in a root post or a reply. Link cards give a great deal more context than the bare URL alone does – in showing the title, description and photo for a linked article, the link card conveys a lot of useful information and helps the user decide if they want to visit the link. And media embeds allow the user to actually view or listen to the content immediately, without having to open a browser or another app. So I think that making embeds invisible in replies by default would be a significant loss of functionality and user value compared to the current Threaded mode. I also think it would cause a lot of confusion. Consider the following reply chain where, as is often done, users have chosen to remove the links after the embed has been added:
If link cards and media embeds are made invisible by default in Threaded mode then those users would only see this:
The YouTube embed in the root post still shows up fine, but in the replies only the text is visible. This would be pretty confusing, especially as users in Linear mode would be able to see the embeds fine but those in Threaded mode wouldn't. So, as I'm sure you've gathered, my opinion is that making embeds invisible in replies by default is really not a good idea. I want to emphasise, though, that I think you've done a great job overall with this PR and I would really like to see a version of this merged in (I'm looking forward to using it myself.) Therefore, instead of my previous assumption/suggestion of adding this as a separate mode alongside the existing Threaded mode – which, on reflection, would be less than ideal because it would add significant additional complexity – what about just making the behaviour of hiding embeds in replies optional? Threaded mode is already classed as Experimental, so beneath the existing checkbox which enables it you could just have another checkbox saying something like:
By doing it this way, users who like and want to retain the current behaviour of embeds being shown in replies could leave the setting off, and users who prefer Threaded mode in the most compact form possible could enable that option and choose to hide all external embeds in replies. |
Sorry — I'm not really seeing the difference between these suggestions? Wouldn't either of these result in a new configuration option in the dropdown, and the difference in what you're suggesting is about precisely what behavior is being toggled by that option? Either suggestion requires pretty significant additional complexity by way of wiring this behavior up to a setting, and I strongly prefer not to block this PR on that additional code, to the point where I would prefer to remove this behavior before making it optional.
"confusing" mostly means "unfamiliar", confusing behaviors are introduced all the time — I didn't realize I could long-press on the profile image in the bottom bar to switch accounts on mobile for weeks, for instance. I don't believe this will be so confusing as to mislead people. It has occurred to me, though, that it's possible for a post to delete the URL after an embed is added to the post, and that is an edge case that should be handled before merging this PR. I'll await further information about what requirements must be met to get this PR approved before investing further energy, though. |
"Thread view" for replies hasn't quite hit the mark for me — there's a lot of vertical screenspace reserved for controls and link embeds, which leaves room for improvement in my eyes on "skimmability". If I'm reading the replies, I'm usually looking for broad-strokes.
This makes a couple of changes:
I'm opening this as a draft PR because there are some bugs I want to sort out before calling it fully ready, specifically
Linear view collapses as well, which was not my intention"never collapse text" is not scoped to only thread view, which was my intention.Here's a before/after comparisonSee later comment for updated screenshotAnd here's a recording of the collapse experience
Screen.Recording.2025-01-18.at.4.46.43.PM.mov