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

Replace scrollable_positioned_list with super_sliver_list #1361

Merged
merged 2 commits into from
May 14, 2024

Conversation

hjiangsu
Copy link
Member

@hjiangsu hjiangsu commented May 9, 2024

Pull Request Description

For some background, I'm in the process of refactoring the comment list to align more with our current conventions of using Slivers for performance reasons. However, I encountered one issue where scrollable_positioned_list does not have a Sliver version.

Because of this, I started to look into other packages and stumbled upon super_sliver_list. This package allows us to scroll to a given index, and from my testing, seems to work fairly well. Additionally, it exposes a few more additional functions that could be useful (including the ability to specify the alignment when scrolling to a given index)

This PR essentially replaces the usage of scrollable_positioned_list with super_sliver_list. Additionally, I've adjusted the logic for the comment navigation FAB. It now keeps track of the index internally which hopefully simplifies some logic.

@micahmo I'm not sure if the changes made to the comment navigation FAB breaks anything, but do let me know (since you were mainly the one who implemented the scroll-to-index functionality)!

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?

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.

This looks great! I had stumbled upon super_sliver_list too and it totally makes sense as the next logical step to use the slivers for the comments.

In addition, I think keeping track of the index internally rather than through a bloc event is a huge upgrade, because there were a number of times that the state wouldn't be right after jumping (because it wouldn't emit everything correctly).

The best part of this change, which might not have even been intentional, is that it seems to fix #588!!

Note that this PR will have some conflicts with #1319.

@hjiangsu
Copy link
Member Author

In addition, I think keeping track of the index internally rather than through a bloc event is a huge upgrade, because there were a number of times that the state wouldn't be right after jumping (because it wouldn't emit everything correctly).

There are some situations where we might need to override the index (such as when we jump to a specific comment, or when performing search) which is why I kept navigateCommentIndex in post bloc. However, for everything else, I do agree that storing it locally simplifies a lot of the logic!

Note that this PR will have some conflicts with #1319.

I'll see what I can do here to mimic the behaviour from #1319, since we don't have access to itemScrollController anymore!

@micahmo
Copy link
Member

micahmo commented May 11, 2024

I'll see what I can do here to mimic the behaviour from #1319, since we don't have access to itemScrollController anymore!

Thanks!! The main things are to delay the scrolling until the comments are built, and to calculate the height of the bottom spacer based on the height of the last comment so it can always be scrolled to the very top even if short.

@hjiangsu
Copy link
Member Author

I'll go ahead and merge this in now that #1319 is merged in. Let me know if you encounter any issues running with this change!

@hjiangsu hjiangsu merged commit e436f24 into develop May 14, 2024
1 check passed
@hjiangsu hjiangsu deleted the misc/use-super-list branch May 14, 2024 15:17
@micahmo
Copy link
Member

micahmo commented May 14, 2024

I just found the first problem with this. If you navigate down (e.g., to the 2nd comment), and then manually scroll up, navigating down should bring you back down to the 2nd comment. However, because of the internal state, it navigates you all the way to the 3rd comment.

The nice thing about scrollable_positioned_list (and I'm not sure if super_sliver_list has this functionality) is that it always understood where we were in relation to the items. So it could always bring the "next" item to the top, regardless of the current index.

Here's where the logic was to find the current index, if it helps.

https://github.com/thunder-app/thunder/pull/1361/files#diff-864f8928f394bbff7bfafd030900915382650e584d9c4ffb3ebefa4efba42b90L87

@hjiangsu
Copy link
Member Author

Interesting, I didn't know that behaviour existed! There might be a way to do this, but I would have to look further into how super_sliver_list works. Thanks for finding this 😅

@micahmo
Copy link
Member

micahmo commented May 14, 2024

Thanks!! I'm pretty sure it's how most people will expect the jump feature to work. Jump down or up to the next top-level comment. (It occurred to me that the same problem also occurs when jumping upward.) It needs to be relative to where we've scrolled, not absolute.

@micahmo micahmo mentioned this pull request Jun 2, 2024
17 tasks
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