-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Add Scroll to top feature in comments view #11654
Conversation
Thank you for your PR. Due to the influx of new PRs review might take some time. Please let me know if you are an ANU student for internal statistics and because I am writing my master thesis about open source contributions as part of learning computer science and computer science education at university. |
Hi @TobiGr, thank you so much for your quick reply. Yes I am an ANU student, I hope your thesis goes well :) |
I prefer a fab with top arrow icon :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A FloatingActionButton
with an upward-facing arrow icon would be a better choice for this purpose. Also, the button could be hidden if the scroll position is already at the top of the list.
android:layout_alignParentBottom="true" | ||
android:layout_marginEnd="10dp" | ||
android:layout_marginBottom="10dp" | ||
android:onClick="onClick" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The onClick
attribute is redundant since you're using setOnClickListener
to listen for click events.
@@ -28,6 +29,7 @@ public class CommentsFragment extends BaseListInfoFragment<CommentsInfoItem, Com | |||
private final CompositeDisposable disposables = new CompositeDisposable(); | |||
|
|||
private TextView emptyStateDesc; | |||
private Button scrollToTopButton; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The scrollToTopButton
is not referenced anywhere other than the initViews
method. You should remove the field and make it a local variable instead.
Quality Gate passedIssues Measures |
NewPipe is currently in maintenance mode and is not accepting PRs for new features, as explained on top of the README, especially niche features like this one. The refactor branch contains a scrollbar which is equivalent to having a "scroll to top" button, you can download an APK from here: https://github.com/TeamNewPipe/NewPipe-refactor-nightly. Thank you anyway for your contribution! |
What is it?
Description of the changes in your PR
Before/After Screenshots/Screen Record
Before:
After:
Fixes the following issue(s)
APK testing
The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR. You can find more info and a video demonstration on this wiki page.
Due diligence