Added bottom main-tabs feature #9719
Conversation
TobiGr
added
feature request
There was a problem hiding this comment.
I don't think it looks so good when the mini player is open. Maybe it would be better if you made the tab layout colored like the app background, when it is at the bottom? When at the top, it should still be service-colored.
How does the YouTube app lay things out when the mini player is open?
Other than that, looks simple enough and mostly good to me. Thank you!
| @@ -106,6 +110,7 @@ public void onResume() { | |||
| } else if (hasTabsChanged) { | |||
| setupTabs(); | |||
| } | |||
| updateTabsPosition(); | |||
There was a problem hiding this comment.
Shouldn't you do this only if tab settings were actually changed, to avoid useless recalculations?
app/src/main/java/org/schabi/newpipe/fragments/MainFragment.java
Outdated
Show resolved
Hide resolved
|
I think what makes it look odd isn't the colour (could be too), but the placement when the player is minimized. Bottom bar should always stay at the bottom, mini player shouldn't move it from there, but go on top of it. Then it would also look uniform with the system navigation bar. |
|
Yeah, that's true. But moving the bottom player to a different position in the UI would be pretty difficult I think. |
Co-authored-by: Stypox <stypox@pm.me>
This comment was marked as outdated.
This comment was marked as outdated.
No, you would still have two nested tab layouts which is not a good idea. Also, this is unrelated to this PR. |
app/src/main/java/org/schabi/newpipe/fragments/MainFragment.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/fragments/MainFragment.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/fragments/MainFragment.java
Outdated
Show resolved
Hide resolved
|
Thanks! There are still some comments you didn't solve (also from the previous round of reviewing). Also, please mark comments you have solved as "Resolved", in order for you to keep track of them in an easier way. Also, did you read the review top content?
|
|
@Stypox I tried to put the tab layout below the mini player but it hasn`t worked yet. Maybe you have a hint how to solve the problem? |
This comment was marked as off-topic.
This comment was marked as off-topic.
There was a problem hiding this comment.
I pushed two commits fixing up the comment that were not solved yet (i.e. the one about only updating when needed). I also improved the code in updateTabLayoutPosition to make it simpler to grasp. I tested on my device and two emulators (API 22 and API 33) and everything worked as expected. Don't worry about the player position, it's not the best but it would be too difficult to change. Do you think my changes are ok? Thanks again!
|
Kudos, SonarCloud Quality Gate passed! |
This comment was marked as spam.
This comment was marked as spam.
What is it?
Description of the changes in your PR
Before/After Screenshots/Screen Record
Fixes the following issue(s)
Due diligence