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

Make all buttons in player have selectable item background borderless #7042

Merged
merged 1 commit into from
Sep 18, 2021

Conversation

Stypox
Copy link
Member

@Stypox Stypox commented Sep 2, 2021

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

I replaced all selectableItemBackgrounds in the player with selectableItemBackgroundBorderless, so that the press animation looks better. I also did this to text buttons (fit-fill-zoom, captions, resolution, time), and I think it is better than non-borderless even though there are some small issues: the text changing when tapping fit-fill-zoom and when opening the resolution menu (MPEG-4 is added to the string) causes some artifacts in the animation, and the captions button, since it contains a long text, causes a big circle. Please provide your feedback :-)
I also ran CTRL+ALT+L to auto format and fixed some RTL warnings.

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

@TobiGr TobiGr added the player Issues related to any player (main, popup and background) label Sep 2, 2021
@litetex
Copy link
Member

litetex commented Sep 3, 2021

Code looks good 👍

However the background when tapping the button gets a bit too big on some buttons in my opinion:
grafik
Personally I was fine with the rectangular background-buttons 😄

@opusforlife2 opusforlife2 added the GUI Issue is related to the graphical user interface label Sep 4, 2021
@opusforlife2
Copy link
Collaborator

Before/after screenshot, pls.

@AudricV
Copy link
Member

AudricV commented Sep 4, 2021

I think we should keep the rectangular background for ratio, subtitles and quality selector commands.

Except buttons with long text or text that changes on click
@Stypox
Copy link
Member Author

Stypox commented Sep 10, 2021

Before/after screenshot, pls.

I would have to upload something like twenty screenshots, one for each button, so I don't think it would be much useful in this case.

I think we should keep the rectangular background for ratio, subtitles and quality selector commands.

I did this @TiA4f8R @litetex

Copy link
Member

@litetex litetex left a comment

Choose a reason for hiding this comment

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

LGTM

I'm personally okay with the changes 😄

@opusforlife2
Copy link
Collaborator

one for each button

Just one example would be enough, I think? It's only needed to get an idea of what the change looks like. I can't figure that out from your description.

@Stypox
Copy link
Member Author

Stypox commented Sep 11, 2021

Before After

@opusforlife2
Copy link
Collaborator

Thanks, @Stypox. After seeing the screenshot, I agree with TiA4f8R. The icons certainly look better with a circular background, but text menus should still have a rectangular one.

I think we should keep the rectangular background for ratio, subtitles and quality selector commands.

@Stypox
Copy link
Member Author

Stypox commented Sep 11, 2021

Ok, that's done (only the speed selector still has a circular background), thanks for the feedback ;-)

@Stypox
Copy link
Member Author

Stypox commented Sep 18, 2021

(just tested on API 19 emulated and it works)

@Stypox Stypox merged commit 8272b25 into TeamNewPipe:dev Sep 18, 2021
This was referenced Oct 2, 2021
@Stypox Stypox deleted the player-borderless-background branch August 4, 2022 09:49
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 player Issues related to any player (main, popup and background)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants