Automatic collapse child comments

[schmeat]

Checklist

• I have read CONTRIBUTING.md
• I have described what this PR contains
• This PR addresses one or more open issues that were assigned to me:
  - list issue(s) here
• If this PR alters the UI, I have attached pictures/videos

Pull Request Information

About this Pull Request

Adding a feature that automatically collapses child comments when a user loads a page. A feature toggle is added to the settings with the default being off.

Screenshots and Videos

Screenshot	Screenshot

Video
https://streamable.com/b0h7s2

Additional Context

Other apps such as Apollo have this feature and thought it would be a good addition. Please let me know if I should change naming conventions. The icon in the settings page is missing too, was not sure what the best one is available for that.

Apollo did have different UI for this feature, it would say [X amount of replies] under each parent comments and would load comments if a user clicked that. I can add that functionality but just did core functionality for now while I wait for feedback.

[Sjmarf]

Thanks for the PR! This is a useful feature.

Regarding the settings icon, I think "arrow.down.and.line.horizontal.and.arrow.up" would be a good choice. This is what we use elsewhere in the app to represent collapsing a comment.

I did notice a small issue. When a comment is collapsed, its children should be hidden completely. However, when a post is first opened with the new setting enabled, all of the children are visible, even those whose parents are collapsed.

Here's what it look like currently:

Here's what it should look like:

The correct appearance can be reached by expanding and re-collapsing each top-layer (orange-colored) comment.

To hide comments completely, you can set the isParentCollapsed value to true.

[schmeat]

Good catch @Sjmarf , I'll check to see why that collapse behaviour is happening. I'll also update the icon too. Thanks

[schmeat]

Adding the feature flag to isParentCollapsed produces this result:

I do find that it looks better this way and the user can see amount of replies in the bottom portion of the comment box. However, clicking on it collapses the comment which means the user has to press the comment twice to see child comments. Looking at ways to work around that right now, any suggestions?

I also need to put isParentCollapsedto true on both of the available comment blocks in order to produce this result, doing it on only one seems to just produce the same behaviour as before.

[Sjmarf]

Hmm yeah that's a little tricky.

I think we'd need to have isParentCollapsed set to true for all comments further down than the orange-colored reply, but set to false for the top-level comment and the orange-colored reply. This should be doable by calculating the depth of the comment (this is done already in the initialiser of HierarchicalComment) and only setting isParentCollapsed to true for comments with a depth of 2 or more.

Maybe instead of determining isCollapsed and isParentCollapsed before the HierarchicalComment is initialised, we could do it inside of the HierarchicalComment initialiser? That way, we could access the depth value that is calculated in the initialiser.

[schmeat]

Ok updated logic:

https://streamable.com/fevk7o

[Sjmarf]

Looks good! I like the new settings location.

[schmeat]

For some reason every time I try to revert the enum, it discards the change and makes a blank commit. Unsure why that's happening, maybe due to git hooks? Not sure

[EricBAndrews]

For some reason every time I try to revert the enum, it discards the change and makes a blank commit. Unsure why that's happening, maybe due to git hooks? Not sure

This is due to the Swiftformat hook. Icons contains nothing but static members--it's basically a namespace, except that Swift doesn't have namespaces. It therefore doesn't make sense for it to be instantiable, hence declaring it as a caseless enum.

[schmeat]

I actually think I can implement the Apollo style collapse (where it displays n replies below the comment box) sooner rather than later. I have the functionality down just working on UI. Please hold off on merging.

[Sjmarf]

Yep, that fixed it!

This introduced a bug, though. If you click "show n replies", then go to a user profile, then click "back", the comment tree is collapsed again but the "show n replies" button is hidden.

Before the most recent fix, the "Show n replies" button would be (correctly) shown in this situation.

[schmeat]

That was because I removed the onDisappearsince it should only be collapsing in certain contexts, but still not sure what's causing it. Anyways I readded that onDisappear code and it should work now

[Sjmarf]

Because the comments section uses a LazyVStack, using .onDisappear makes the "Show n replies" button appear again on an expanded comment if you scroll down the comments and then back up.

[schmeat]

@Sjmarf Not sure what the best solution to this would be. I tried to set isCommentReplyHidden in .onAppear instead, but it seems like when we navigate back to the post from the user profile, isCollapsedis not being set correctly in HierarchicalComment. I'm unsure what method is actually collapsing it still, since it's not hitting any of my current collapse methods.

I tried debugging and it's showing as retaining the hidden variable but not resetting collapse. I guess it's particularly tricky in this case because isCollapsed being false can be when the child comments are hidden and not hidden.

ID: 3350207, isCollapsed: false, isCommentReplyHidden: false, number of children: 8
ID: 3350207, isCollapsed: false, isCommentReplyHidden: true, number of children: 8

Would that be an acceptable bug to ship with? Will take me some more time to investigate the root cause of it and I'm not sure how much time I can dedicate this week. If you have a hunch as to why this behaviour is happening please let me know.

[Sjmarf]

I'll take a look at it this weekend and see if I can find the issue 👍

[Sjmarf]

Looks like it's caused by the .task in ExpandedPost on line 85. Adding an if statement fixes it

.task {
    if commentTracker.comments.isEmpty {
        await loadComments()
    }
}

[schmeat]

Thanks so much for checking, I've added those changes now!

[Sjmarf]

Looks good to go!

[schmeat]

Is there something I need to fix to get it merged? I noticed some build failures but logs say something about access

[Sjmarf]

Every time you make a change, the checks need to re-run. For organisation members this happens automatically, but for everyone else it has to be approved by an organisation member :(

Also occasionally the checks do fail for no apparent reason and we just have to re-run them.

Feel free to merge this whenever you like (if you're able to? If not lmk and I'll merge it).

[schmeat]

Okay sounds good. I don't think it lets me merge unless all checks are passed.

[Sjmarf]

Huh, I see the Error: Resource not accessible by integration too now 🤔

I've not seen that before

[schmeat]

Not sure what the build settings for this project are, but maybe this is it:

kishikawakatsumi/xcresulttool#691 (comment)

We need pull_request_target if the repository is forked.

[Sjmarf]

Thanks - Jake's put a PR up to fix it, once that's in I'll rerun the checks 👍

[EricBAndrews]

Builds and checks pass locally--we're still getting our CI stuff worked out for forked builds, so once this PR is merge-compatible I'll run the checks again locally and bypass CI to merge

[schmeat]

Sorry for the delay, busy week. Should be good to go now.

[EricBAndrews]

Build and test work locally. I'm bypassing branch protections to merge without CI; if this breaks dev that's on me.