-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Add option to display channel groups as list instead of grid #9207
Add option to display channel groups as list instead of grid #9207
Conversation
…ticalItem in line 26
…ing on refreshing
…s in SubscriptionFragment.kt
Why did you create two PRs? Which one should be merged, this one or #9204? Once you create a PR you can easily just push more commits to it, there is no need to create a new PR each time we ask you to make changes |
@Stypox this one,. sorry for the confusion. I panicked because of a deadline |
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.
I pushed a commit that fixes a couple of implementation and logic issues. Some of the code not written by you (but that was there before this PR) had some problems that needed to be solved for this PR to work well. I also changed some of your code after some things around it changed: an important change is that I moved the list view state handling to the view model (which is the usual thing to do in Android). Take a look at the code in the commit for more information.
Please revert the changes to 5.json
and FeedGroupEntity.kt
. I don't know where those came from. Then I can re-review the code and this can be merged. Thank you for this neat and needed feature!
Here is a testing APK: app-debug.zip
While we're at it, I think the New button should be at the top. Edit: Cosmetic issue: there is flickering in between switching states. If it can't be avoided, maybe an animation to smooth it out? |
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.
@opusforlife2 done. I'm merging this if no issues arise in this APK (please test!): app-debug.zip
@Stypox Hi, I just ran through the APK and found an issue where the subscribers list would slide from its original position to the bottom everytime someone goes to another page and goes back to subscribers. |
If you want to try to fix it go ahead, otherwise I will probably take care of it later or in the following days |
9d76f96
to
ea875c5
Compare
Looks great! One last thing: can the tap area of the layout change button be increased? I'm seeing some hits and misses trying to tap it. It's close enough to the edge of the screen that my phone cover is interfering somewhat. |
@opusforlife2 done
@cern1710 btw, I could not reproduce. Could you provide more precise reproduction instruction, or a video? |
Perfect! The button can be easily tapped now. |
The subscription page (along with its channels) will fall down from the top, which has probably something to do with the animations? Perhaps this can be solved by another PR |
Oh. I got it. The exact same thing happens when you toggle from list layout to grid. The Subscriptions section moves down in a slow animation, which makes it overlay other stuff for a noticeable duration. |
…' into list-view-alt-alt-implementation
Kudos, SonarCloud Quality Gate passed! |
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.
I tried to mess around with the animations, but I could not find a way to make everything work correctly. So yeah, let's stick with this implementation. Thanks!
What is it?
Description of the changes in your PR
Before/After Screenshots/Screen Record
Fixes the Following issue:
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