thunder icon indicating copy to clipboard operation
thunder copied to clipboard

Refactor Post/Comment page

Open hjiangsu opened this issue 1 year ago • 3 comments

Pull Request Description

This is a draft PR which refactors the existing post page (and comments). This is still very early in development and does not have all the features that the current post page has. Since this is a large re-architecture/refactor, I've opted to do the following:

  • I introduced a new toggle in the Debug settings page to enable experimental mode. Thunder will need to be restarted in order to use the experimental features.
  • When experimental features are enabled, navigating to the post page will use the new post page. Otherwise, it will continue to use to current post page.
  • I would like to slowly bring the refactor up to parity with the current post page. I'm thinking we can do this with a series of PRs. The benefit with hiding this under an experimental toggle is that we can merge the post page when its still in an incomplete stage without affecting the existing implementation

The current state of the post page allows you to:

  • View the post information immediately upon opening the post
  • View the comments associated with the post
  • Refresh the comments
  • Switch the sort type for the comments

Current TODOs before this PR is ready:

  • [x] Add back the additional top app bar actions (creating cross-post, viewing post source, copying post content)
  • [x] Fix post actions interactions
  • [x] Fix comment action interactions
  • [ ] Fix comment not refreshing on action

Future TODOs (to come in separate PRs):

  • Bring back post page FAB
  • Bring back comment navigation
  • Allow comment navigation to navigate between top level comments, and also child comments
  • Bring back comment search - with the ability to scroll to specific child comments

Because of this, any future PRs (enhancement or new features) related to post/comment page should be targeting the new post page rather than the old one (PRs that fix existing issues with the current post page will still be accepted). This generally only includes the following (and anything marked as deprecated):

  • PostPageSuccess, PostPage (old) -> to be replaced by the new PostPage
  • CommentSubview -> To be replaced by the new PostPage

Some additional notes on the refactor

This refactor should improve the overall scroll performance of the post/comments page (with the switch to slivers), and should also fix issues where we can only navigate to top level comments. To do this, I changed up the implementation for how we store comments. Comments are stored as a tree structure, but they are flattened before we display them on the post page. This makes it easier to scroll to a specific index because it's no longer a recursive widget tree of comments.

  • We can do some cool things here like long-press to go to the next parent comment, or short press to go to the next comment, etc!

Any thoughts on this approach @micahmo?

Issue Being Fixed

Issue Number: N/A

Screenshots / Recordings

Checklist

  • [ ] Did you update CHANGELOG.md?
  • [ ] Did you use localized strings where applicable?
  • [ ] Did you add semanticLabels where applicable for accessibility?

hjiangsu avatar May 11 '24 18:05 hjiangsu

I think this is a great approach! A lot of the things you mentioned are common issues/requests that come up, so I think this will be great in the long run! And by targeting it to an entirely new set of files and a feature flag, we can still merge any existing PRs and even put this out there for early testing.

micahmo avatar May 13 '24 15:05 micahmo

Thanks for the feedback! This should almost be ready to be merged in (pending the current TODOs) 😄

hjiangsu avatar May 13 '24 20:05 hjiangsu

Once this is in (or maybe as part of these changes), can we put the UnifiedPush stuff behind the same experimental flag? That way it'll be easier for us to test on non-debug builds (and without code changes).

micahmo avatar May 13 '24 21:05 micahmo

Alright, I think I've got this up to the point where we can merge it in. I tested most things described above, and they seem to work (at a basic level). @micahmo, no need to do a code review since there's been a lot of changes here (but feel free to do so if you'd like!)

hjiangsu avatar May 31 '24 00:05 hjiangsu

I just did a very brief skim through of the code, but like you said, this will be best reviewed via testing. Feel free to merge this in and I will try to daily drive with the experimental flag on. I will mention any quirks I find (aside from things that you've already called out as TODOs)!

micahmo avatar May 31 '24 03:05 micahmo

Here are my observations so far. Apologies if any of these are things you already knew about.

  • Collapsing comment threads doesn't have any animation.
  • Collapsing a thread with only deferred comments beneath it does nothing.

https://github.com/thunder-app/thunder/assets/7417301/7f0b3b76-9df4-4c41-8fb7-a028f3cd69e8

micahmo avatar May 31 '24 15:05 micahmo

Collapsing comment threads doesn't have any animation. Collapsing a thread with only deferred comments beneath it does nothing.

Hmm, weird. I can't seem to reproduce the issue on my end

https://github.com/thunder-app/thunder/assets/30667958/db3e199a-79df-4ca5-b6a4-253351d143df

Edit: Looking at the video, it seems like it tries to collapse it (the reply count appears and disappears)

hjiangsu avatar May 31 '24 16:05 hjiangsu

Also tried on Android emulator and it seems to work. This is based on the latest develop build:

https://github.com/thunder-app/thunder/assets/30667958/855bf99d-d47b-4a4a-9cad-e52be5db86fd

hjiangsu avatar May 31 '24 16:05 hjiangsu

Sorry, I should've clarified! I have the "Hide Parent Comment when Collapsed" setting off, but I still expect the deferred comments ("Load x more replies...") to be hidden when I collapse the parent. At least that's how the old post page worked. 😊

micahmo avatar May 31 '24 16:05 micahmo

One more super small thing I noticed: the indentation of the first nested comment looks like it's twice as wide as it should be. You can see the difference when comparing to the indentation of the deferred comments.

image

micahmo avatar Jun 01 '24 00:06 micahmo