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

Re-introduce comment navigator for new post page #1496

Merged
merged 2 commits into from
Jul 16, 2024

Conversation

hjiangsu
Copy link
Member

@hjiangsu hjiangsu commented Jul 14, 2024

Pull Request Description

This PR adds back the comment navigator for the new experimental post page. The general logic remains similar to the current implementation, but with one behavioural change. Because the comments are now flattened, the comment navigator will navigate to each child comment (as opposed to the next parent comment). A separate PR will be made to allow navigation between top-level comments using this navigation.

With this, I've also added back the ability to scroll to the last comment (somewhat). There are some quirks here due to the transition to slivers. Slivers don't have a RenderBox associated with them, so fetching the height of the sliver is less trivial (from my testing). I do think I have it at a good point, but will need to do more testing on it to be sure.

More specifically, this PR fixes the following:

  • Bring back comment navigation
  • It should be possible to scroll the last comment to the top of the screen.

Issue Being Fixed

Issue Number: #1413

Screenshots / Recordings

Checklist

  • If a new package was added, did you ensure it uses an appropriate license and is actively maintained?
  • Did you use localized strings (and added appropriate descriptions) where applicable?
  • Did you add semanticLabels where applicable for accessibility?

@micahmo
Copy link
Member

micahmo commented Jul 14, 2024

Awesome, I will try daily driving this for a little while!

Copy link
Member

@micahmo micahmo left a comment

Choose a reason for hiding this comment

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

The code looks good!

So far from my testing it's working fine! Like you said, it will require another change to only navigate between top-level comments.

As far as scrolling to the bottom, it seems to be working fine, although it allows the "you've reached the bottom" message to be scrolled to the top, rather than the last comment. I'm not sure if that was intentional, and it might not be an easy fix (since each comment is a separate widget, you'd have to add keys to each of the comments under the last top level comment and combine all their heights), but either way I think it's fine!

@hjiangsu
Copy link
Member Author

As far as scrolling to the bottom, it seems to be working fine, although it allows the "you've reached the bottom" message to be scrolled to the top, rather than the last comment. I'm not sure if that was intentional, and it might not be an easy fix (since each comment is a separate widget, you'd have to add keys to each of the comments under the last top level comment and combine all their heights), but either way I think it's fine!

I left it there somewhat intentionally for the reasons that you mentioned (it would be difficult to calculate the full extent of the last comment, especially if it has children). We can leave it as is for now and adjust it when needed 😁

@hjiangsu hjiangsu merged commit 14f79e9 into develop Jul 16, 2024
1 check passed
@hjiangsu hjiangsu deleted the feature/post-page-comment-navigator branch July 16, 2024 16:09
@micahmo
Copy link
Member

micahmo commented Aug 1, 2024

@hjiangsu I've noticed a small bug from this change which is that, when you press down for the very first time or any time you are scrolled above the top comment, it always goes to the second comment rather than the first.

I am going to add a note to #1413 as well.

qemu-system-x86_64_p8L9RwvTb5.mp4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants