-
-
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
Migrate comment fragments to Jetpack Compose #11060
base: refactor
Are you sure you want to change the base?
Migrate comment fragments to Jetpack Compose #11060
Conversation
c77ea74
to
c106fb0
Compare
ce7c3ee
to
64a92bd
Compare
c834fd0
to
8ee1817
Compare
da0c0c9
to
7f7faac
Compare
31f1fcf
to
e973ce1
Compare
3ce32b0
to
2049c81
Compare
bace455
to
10dd571
Compare
e608cc5
to
ebcfcc4
Compare
# Conflicts: # app/build.gradle
7bc02bd
to
d3a6991
Compare
838c18f
to
f9340ae
Compare
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.
app/src/main/java/org/schabi/newpipe/ui/components/video/comment/CommentSection.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailFragment.java
Outdated
Show resolved
Hide resolved
42cb443
to
b1add13
Compare
Hi, thank you so much for your work, it's terrific! Since your rewriting this part, do you think it would be a good chance to tackle #7191? Support in extractor has been added if I'm not wrong. Also, perhaps an m3 coloured scroll handle would blend in better. I would comment on a few more stuff, but they're things that were there before the migration (replies count on the bottom sheet header, copy on long press), so I guess you'll add them back eventually. |
@tsiflimagas we are happy about every reviewer, so if you have any comments on the PR leave them here😉 |
As far as I can tell, it hasn't been implemented yet, seeing as that issue is still open.
I'm using a third-party library for the scrollbars, since first-party support is yet to be implemented. |
Thank you!
The issue is open on the app side, since it hasn't been implemented in the app. If I'm not wrong this PR has added the necessary support in the extractor.
By looking at the library's repo, it seemed to me that it offers the ability to edit scrollbars colour thus set it to an adaptive one, but maybe I got it wrong. Also, it seems that none of the migrated fragments follows the black night theme. Perhaps it is planned to revamp app theming, or would it make sense to make these fragments follow app's theme as they it before? |
Thanks, I missed that one.
Yeah, it's possible to change the scrollbar colour. How does red (the primary colour) sound?
Yeah, the black theme will need to be implemented. |
It's good I guess. The app hasn't been migrated to material You anyway, so it looks fitting. But you may choose what you find better, as the author of the PR. |
97146ea
to
62d4044
Compare
Quality Gate passedIssues Measures |
What is it?
Description of the changes in your PR
Before/After Screenshots/Screen Record
Comments
Comment replies
Comments
Comment replies
Screen_recording_20240708_162549.mp4
Relies on the following changes
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