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

Migrate about activity to Jetpack Compose #11282

Open
wants to merge 16 commits into
base: refactor
Choose a base branch
from

Conversation

Isira-Seneviratne
Copy link
Member

@Isira-Seneviratne Isira-Seneviratne commented Jul 14, 2024

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

  • Migrate the about activity to use Jetpack Compose and remove unused layout files.
  • Add a ScaffoldWithToolbar composable for future reuse in refactoring.
  • Remove the ViewPager2 library as its only use was in the about activity.
  • Use AboutLibraries to dynamically display the list of external dependencies of the project.

Before/After Screenshots/Screen Record

  • Before:
    Screenshot_20240714_073717_NewPipe
    Screenshot_20240714_073722_NewPipe
    Screenshot_20240714_074847_NewPipe

  • After:
    Screenshot_20240715_084245_NewPipe About-Compose
    Screenshot_20240823_140621_NewPipe Compose-all
    Screenshot_20240823_140636_NewPipe Compose-all

Fixes the following issue(s)

  • Fixes #

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

@github-actions github-actions bot added the size/giant PRs with more than 750 changed lines label Jul 14, 2024
@opusforlife2
Copy link
Collaborator

OMG finally a JC PR where there is a visible change in the UI.

@wb9688
Copy link
Contributor

wb9688 commented Jul 14, 2024

Dark red text for the tabs isn't quite ideal though, but I suppose the PR is still WIP. Can you please mark WIP PRs as draft next time?

Great work again with regards to doing yet another Jetpack Compose migration.

@wb9688 wb9688 marked this pull request as draft July 14, 2024 21:08
@wb9688
Copy link
Contributor

wb9688 commented Jul 14, 2024

Now I looked a bit at the code and I think the list of libraries is definitely outdated, so that should be fixed as well.

@TobiGr
Copy link
Member

TobiGr commented Jul 14, 2024

Regarding outdated libs: #11202 should be implemented once this is merged.

@Isira-Seneviratne
Copy link
Member Author

OMG finally a JC PR where there is a visible change in the UI.

The comments PR has visible changes as well.

@Isira-Seneviratne
Copy link
Member Author

Isira-Seneviratne commented Jul 14, 2024

Dark red text for the tabs isn't quite ideal though, but I suppose the PR is still WIP. Can you please mark WIP PRs as draft next time?

Great work again with regards to doing yet another Jetpack Compose migration.

It's mostly completed, it's just that one detail remaining.

Also, we might want to review how the theme should look. This should help: https://material-foundation.github.io/material-theme-builder/

@Jean-BaptisteC
Copy link

I have noticed 2 things:

  • A scroll bar is present with weird color (in my case it's blue)
  • Maybe it's better to have a transparent android app bar?

@Isira-Seneviratne
Copy link
Member Author

I have noticed 2 things:

  • A scroll bar is present with weird color (in my case it's blue)

The scrollbar is provided by a third-party library (Compose does not currently support scrollbars).

  • Maybe it's better to have a transparent android app bar?

Sounds good.

Copy link

sonarcloud bot commented Aug 11, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
9.2% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

@Isira-Seneviratne
Copy link
Member Author

Regarding outdated libs: #11202 should be implemented once this is merged.

That library includes a composable that automatically displays the external dependencies, so I used that in this PR itself.

# Conflicts:
#	app/build.gradle
#	app/src/main/java/org/schabi/newpipe/ktx/Bundle.kt
#	build.gradle
@TobiGr TobiGr added the GUI Issue is related to the graphical user interface label Sep 2, 2024
Copy link

sonarcloud bot commented Sep 16, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GUI Issue is related to the graphical user interface size/giant PRs with more than 750 changed lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants