-
Notifications
You must be signed in to change notification settings - Fork 69
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
Refactor Post/Comment page #1363
Conversation
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. |
Thanks for the feedback! This should almost be ready to be merged in (pending the current TODOs) 😄 |
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). |
7e69367
to
f0c0e7f
Compare
c89b84f
to
3d0689a
Compare
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!) |
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)! |
93b71eb
to
516eab3
Compare
Here are my observations so far. Apologies if any of these are things you already knew about.
qemu-system-x86_64_G9JKLLDJq0.mp4 |
Hmm, weird. I can't seem to reproduce the issue on my end Simulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-05-31.at.09.05.02.mp4Edit: Looking at the video, it seems like it tries to collapse it (the reply count appears and disappears) |
Also tried on Android emulator and it seems to work. This is based on the latest post-page-refactor.mp4 |
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. 😊 |
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:
The current state of the post page allows you to:
Current TODOs before this PR is ready:
Future TODOs (to come in separate PRs):
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):
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.
Any thoughts on this approach @micahmo?
Issue Being Fixed
Issue Number: N/A
Screenshots / Recordings
Checklist
semanticLabel
s where applicable for accessibility?