-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Implement Compose-based Error Panel , Error UI Model , and Tests for Comments #12404
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
base: refactor
Are you sure you want to change the base?
Implement Compose-based Error Panel , Error UI Model , and Tests for Comments #12404
Conversation
@Stypox Apologies for the fairly large diff—thought it would make sense to add a new Compose-based ErrorPanel and have it wired up end-to-end as a first step towards migration. I’m happy to split it up or tweak anything to fit the overall architecture—just let me know! |
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.
Thank you! I think this PR is the right size, and also you wrote many tests which increase the "PR line count" but do not add reviewing complexity, so there is no need to split it. And thanks for also writing test! I left a few comments to try to make the code more general so it can be used more seamlessly in other places in the app in the future (to replace other error panels in various places).
app/src/main/java/org/schabi/newpipe/ui/components/common/ErrorPanel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/ui/components/common/ErrorPanel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/ui/components/video/comment/CommentErrorHandler.kt
Outdated
Show resolved
Hide resolved
@Stypox I have incorporated your requests - removed the other files and just added one test file. Also I didn't want to add an additional dependency for compose testing but please let me know if you'd like me to! |
app/src/main/java/org/schabi/newpipe/ui/components/common/ErrorPanel.kt
Outdated
Show resolved
Hide resolved
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/ui/components/video/comment/CommentSection.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/ui/components/common/ErrorPanel.kt
Outdated
Show resolved
Hide resolved
...st/java/org/schabi/newpipe/ui/components/video/comment/CommentSectionErrorIntegrationTest.kt
Outdated
Show resolved
Hide resolved
...st/java/org/schabi/newpipe/ui/components/video/comment/CommentSectionErrorIntegrationTest.kt
Outdated
Show resolved
Hide resolved
...st/java/org/schabi/newpipe/ui/components/video/comment/CommentSectionErrorIntegrationTest.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/ui/components/common/ErrorPanel.kt
Outdated
Show resolved
Hide resolved
@SttApollo Tbh I have no qualms with you adding compose testing, because we are going to be adding UI tests very soon anyway. Currently we have no UI tests whatsoever, and I don't really see a downside to having tests for new functionality. @Stypox, thoughts? |
Yeah it would be cool if you added some tests! Would you be ok with opening a new PR after this one is merged, to setup the UI tests infrastructure and add tests for what you added in this PR? Otherwise you can also add UI tests in this PR, let us know what you prefer :-) |
Sure, I can open a new PR after this one is merged. For the UI test infra, I was thinking we could start with Compose instrumented testing—does that align with what you had in mind? Or should we also add Robolectric-based Compose tests and/or snapshot testing early on? |
98b3a7f
to
c9fab2b
Compare
…ented test compatibility
…rPanel.kt Co-authored-by: Isira Seneviratne <[email protected]>
…nt/CommentSection.kt Co-authored-by: Isira Seneviratne <[email protected]>
…nt/CommentSection.kt Co-authored-by: Isira Seneviratne <[email protected]>
- Moved from androidTest to test directory - Removed Android test runner dependencies - Renamed to CommentSectionErrorTest - Addresses PR feedback until Compose testing is in place
c9fab2b
to
e27c7ed
Compare
(read the method's javadoc for why)
What is it?
Description of the changes in your PR
ErrorPanel
component.MaterialTheme.colorScheme
and that all text is centered for visual parity with the legacy design.ErrorUiModel
interface to represent different failure states along withUnableToLoadCommentsUiModel
specific to Comments section.ErrorUiModel
and Integration tests for the two main error states in the comment section:Resource.Error
(initial load failure) andLoadState.Error
(paging failure). Would add Compose testing with approval to add a new dependency.CommentsViewModel
(Resource.Error) andCommentsSource
(LoadState.Error) to ensure proper UI rendering and error model mapping.Before/After Screenshots/Screen Record
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.