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

Use ListAdapter for search predictions. #8719

Merged
merged 3 commits into from
Aug 19, 2022

Conversation

Isira-Seneviratne
Copy link
Member

@Isira-Seneviratne Isira-Seneviratne commented Jul 31, 2022

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

  • Use ListAdapter to handle displaying list data in PeertubeInstanceListFragment, SuggestionListAdapter and PreferenceSearchAdapter.

Before/After Screenshots/Screen Record

  • Before:
    • Video search:
device-2022-07-31-155131.mp4
- Preference search:
device-2022-08-01-061212.mp4
  • After:
    • Video search:
device-2022-07-31-155211.mp4
- Preference search:
device-2022-08-01-061232.mp4

Fixes the following issue(s)

  • Fixes #

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.

Due diligence

@Isira-Seneviratne Isira-Seneviratne marked this pull request as ready for review July 31, 2022 10:29
Copy link
Member

@TacoTheDank TacoTheDank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I like this change, it looks much nicer.

Some comments:

@TacoTheDank TacoTheDank changed the title Use ListAdapter. Use ListAdapter for search predictions. Jul 31, 2022
@Isira-Seneviratne Isira-Seneviratne force-pushed the Use_ListAdapter branch 5 times, most recently from a058b45 to e2e0be4 Compare August 4, 2022 00:31
Stypox
Stypox previously requested changes Aug 4, 2022
Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good changes! I'll let @TacoTheDank complete the review and merge

@triallax triallax added feature request Issue is related to a feature in the app GUI Issue is related to the graphical user interface labels Aug 4, 2022
@Isira-Seneviratne Isira-Seneviratne force-pushed the Use_ListAdapter branch 2 times, most recently from e6ee5b0 to 1c5bdee Compare August 5, 2022 03:22
@TacoTheDank
Copy link
Member

Sorry, I took a small break over the past week to relax after the past semester.

Could this be rebased to remove the dev merge commit?

@Isira-Seneviratne
Copy link
Member Author

Sorry, I took a small break over the past week to relax after the past semester.

Could this be rebased to remove the dev merge commit?

Sure.

@TacoTheDank
Copy link
Member

@Stypox Are we doing a feature freeze until v0.24.0 is released, or can this be merged now?

@Isira-Seneviratne Isira-Seneviratne force-pushed the Use_ListAdapter branch 2 times, most recently from 27dda7e to 72b6fbb Compare August 16, 2022 16:23
@Stypox
Copy link
Member

Stypox commented Aug 16, 2022

No, we have not decided upon a freeze, you can proceed @TacoTheDank

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@TacoTheDank TacoTheDank dismissed Stypox’s stale review August 19, 2022 19:49

Review completed.

@TacoTheDank TacoTheDank merged commit 2089f3e into TeamNewPipe:dev Aug 19, 2022
@TacoTheDank
Copy link
Member

Thanks for this feature!

@Isira-Seneviratne Isira-Seneviratne deleted the Use_ListAdapter branch August 20, 2022 00:48
@Stypox Stypox mentioned this pull request Aug 27, 2022
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issue is related to a feature in the app GUI Issue is related to the graphical user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants