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

Making drawer accesable for easy navigation #3156

Conversation

Hashik-Donthineni
Copy link
Contributor

@Hashik-Donthineni Hashik-Donthineni commented Feb 28, 2020

Fixes #3135
Closes #2358

Additions:

  1. Now Navigation is accessible from both MainFragment, VideoDetailFragment.
  2. Added the "Home" tab to the drawer to reach home (MainFragment) easily.

I tested this rigorously and there are no bugs I can find. And, I find the app very easy to navigate now :)
This is the Debug-Apk for testing: Debug-Apk (Changed the package name for side by side install)

ZIP Version of APK: app-debug.zip

@Hashik-Donthineni Hashik-Donthineni changed the base branch from dev to master February 28, 2020 21:33
@TobiGr TobiGr changed the base branch from master to dev February 28, 2020 21:38
@TobiGr
Copy link
Contributor

TobiGr commented Feb 28, 2020

Thank you, please keep the dev branch. We should look into this soon

@wb9688
Copy link
Contributor

wb9688 commented Feb 28, 2020

You could use icons from https://material.io/resources/icons/, which are iirc included in Android Studio.

@opusforlife2
Copy link
Collaborator

Debug apk please! (If you could rename the package to org.schabi.newpipe.drawernav or something it would be swell!)

@opusforlife2
Copy link
Collaborator

Oh no.

I just realised avently removed the top navigation bar from the VideoDetailFragment, so he had to enable the side drawer anyway. This PR would be made redundant when that one gets merged. :(

Until then, the settings button restored by @Stypox could work as a substitute.

@Hashik-Donthineni
Copy link
Contributor Author

Hashik-Donthineni commented Feb 29, 2020

@opusforlife2 Can you please explain mate? I mean I already implemented the drawer here anyway, right? Why did he remove the top navigation bar and where can we access the drawer if the navigation is gone? The issue is pretty new and I thought the app needs better navigation.

@wb9688
Copy link
Contributor

wb9688 commented Feb 29, 2020

@opusforlife2: You mean his unified player PR? If so, that probably won't be merged for a while, and then this PR would still be useful in the meantime.

@Hashik-Donthineni
Copy link
Contributor Author

I added the Debug-apk, please check.

@opusforlife2
Copy link
Collaborator

opusforlife2 commented Feb 29, 2020

@wb9688 True.

@Hashik-Donthineni as he said, the unified player PR implements the side drawer. But it would be great to have this functionality now instead of possibly a month later (or even more). :)

Gonna test the apk now. Thanks!

Edit: You can upload the apk to GitHub directly. No need to upload it to Google Drive separately. Also, my suggestion to rename the package was so that it can be installed side by side with other debug apks.

@opusforlife2
Copy link
Collaborator

Tested. Looks and works great! The hamburger icon makes it clear that the side drawer is available, and the back arrow's functionality is shifted to Home, so no loss there. You correctly discard the backstack when tapping Home too, just like the erstwhile back arrow. No complaints with this PR!

@Hashik-Donthineni
Copy link
Contributor Author

Hashik-Donthineni commented Feb 29, 2020

@opusforlife2 I uploaded to GDrive because it's easier to check for upload version in Gdrive and we can update the same link with the newer version if we want and I am commuting, sorry that I didn't change the package name, I'll do it when I get the time and update the same link :)

Edit: Changed the package name

@Hashik-Donthineni
Copy link
Contributor Author

@wb9688 Added the home icon as well, used ic_kiosk_local :)

@opusforlife2
Copy link
Collaborator

Bug found! When you tap on the service selector drop down then dismiss it, the Home button disappears. This happens on both Fragments.

@Hashik-Donthineni
Copy link
Contributor Author

@opusforlife2 Fixed the issue :)

@opusforlife2
Copy link
Collaborator

Confirmed. :)

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.

Thank you, this solves many navigation issues! I tested the apk and it works well.

@Stypox Stypox self-assigned this Mar 3, 2020
@Hashik-Donthineni
Copy link
Contributor Author

Hashik-Donthineni commented Mar 3, 2020

@Stypox In my opinion, it's better to make the Home button available on the MainFragment aswell because the user will get accustomed to using and seeing that button there and it'll get into muscle memory. If we keep moving it, I think it'll contribute to bad UX, unknowingly.

Edit: NVM saw your above comment now :)

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.

Code looks good, thank you!
Could you provide another APK? After a last row of testing I think this is ready to go :-D
Btw, you can upload apks on GitHub by zipping them

app/src/main/java/org/schabi/newpipe/MainActivity.java Outdated Show resolved Hide resolved
@opusforlife2
Copy link
Collaborator

I just realised avently removed the top navigation bar from the VideoDetailFragment, so he had to enable the side drawer anyway. This PR would be made redundant when that one gets merged. :(

Update on this. I was mistaken. He didn't add it. That was just me swiping up the paused video from the home screen. The drawer naturally works there. But it doesn't work if you open the video details screen and try there.

So, this PR is much needed, anyway. 👍

@Stypox
Copy link
Member

Stypox commented Mar 5, 2020

@Hashik-Donthineni could you please provide another APK and resolve conflicts?

@Hashik-Donthineni
Copy link
Contributor Author

Hashik-Donthineni commented Mar 5, 2020

@Stypox Done! updated the same link. BTW why is the build.gradle failing (I am talking about merge conflict)?

Edit: Clarified a bit.
Edit2: NVM it's just strings file

@opusforlife2
Copy link
Collaborator

@Hashik-Donthineni I still don't get it. Can you point out where he says that?

@Hashik-Donthineni
Copy link
Contributor Author

Hashik-Donthineni commented Mar 23, 2020

@opusforlife2 Did I get the wrong impression then? Here is what that made me believe so,

IMO it should be kept as it is, some reasons why:

TL;DR: Keep the drawer active in all screens, keep the up icon the way it is now.

How??? The only way is to kill this PR and redirect the up button to the "Home." I don't see any other way.

@opusforlife2
Copy link
Collaborator

Are you saying that the back button in the top bar has to be changed to be able to use the home button in the drawer?

@Hashik-Donthineni
Copy link
Contributor Author

@opusforlife2 sorry what? I am saying we can't use drawer icon and up icon at the same time? not the the home button inside the drawer.

@opusforlife2
Copy link
Collaborator

Oh. That's it? Just use the Up icon instead of the drawer icon. The drawer can be used by swiping from the left anyway. The button is redundant.

@opusforlife2
Copy link
Collaborator

@TobiGr @Stypox In #2907, the video detail fragment currently doesn't have a top bar.

This means that if you are on a screen where the drawer cannot be accessed (I can think of the search fragment, channel fragment and playlist fragment right now), you can't access Settings without navigating a bit. This is not much of a problem for the playlist and channel fragments, as they have the three dot menu still (so you can swipe down the video and access it), but the search fragment changes that to a filter.

I think the drawer should definitely be available in the search fragment, but it could be made available in the other two as well.

@Hashik-Donthineni
Copy link
Contributor Author

@opusforlife2 So, finally you want me to remove the drawer button and preserve the functionality of up button (and restore the up button of course) and make drawer accessible with a swipe.. that right?

@opusforlife2
Copy link
Collaborator

It is already accessible with a swipe. But in a nutshell, yes. I think that is what Mauricio was asking for as well.

@surrodox2001
Copy link

surrodox2001 commented Mar 25, 2020

@opusforlife2 So, finally you want me to remove the drawer button and preserve the functionality of up button (and restore the up button of course) and make drawer accessible with a swipe.. that right?

Is that @mauriciocolli wanted you to do? I think that will be unintuitive as it's not apparent for the users. (user dont know you can swipe to reveal side menu)

@opusforlife2
Copy link
Collaborator

I think that will be unintuitive as it's not apparent for the users. (user dont know you can swipe to reveal side menu)

Agreed. F-Droid users can see the changelog when updating, but Github users can't. But that is for #3100 to solve, not this PR.

@Hashik-Donthineni
Copy link
Contributor Author

Hashik-Donthineni commented Mar 25, 2020

I think that will be unintuitive

Absolutely.

@opusforlife2 This is what I am really confused about.

@surrodox2001
Copy link

Can @Stypox @TobiGr comment on the @mauriciocolli objection to this PR?

@opusforlife2
Copy link
Collaborator

@Hashik-Donthineni It's not your problem to solve, bro. Don't worry about it. ;)

@Stypox
Copy link
Member

Stypox commented Mar 25, 2020

There are some apps where the drawer is hidden but can be accessed from anywhere (e.g. OctoDroid). So this might not be completely unintuitive. Also, it is really likely that at some point the user swipes left by accident, thus finding the hidden drawer. But Material guidelines say that "a modal drawer is always opened by a navigation menu icon".


off-topic

F-Droid users can see the changelog when updating

I never saw a changelog for NewPipe in F-Droid, I thought it was badly configured, but do you see them?

@opusforlife2
Copy link
Collaborator

But Material guidelines say that "a modal drawer is always opened by a navigation menu icon".

That's okay. Other apps usually don't have the need for them on every screen. We do. (Plus Play Store violates this guideline anyway. 🙄 )

I never saw a changelog for NewPipe in F-Droid, I thought it was badly configured, but do you see them?

It has been doing so for quite a few versions. It's just that the links aren't clickable. For that, the user can just further open the changelog link.

@Hashik-Donthineni
Copy link
Contributor Author

Tbh with yall, in my opinion, material design isn't like a law book, we can take what works for our app. Heck, even google's own apps don't obey material design sometimes.

So, what I suggest is keeping the drawer icon and dropping the up button, the drawer can be shorthand for many things like downloads, home, trending, etc. Up button will take you to home, I think drawer gives more functionality. "Home" in drawer still emulates the up button, it's 2 presses away rather than 1. But, it's a good trade given navigation drawer is giving a variety of functionality.

@surrodox2001
Copy link

Tbh with yall, in my opinion, material design isn't like a law book, we can take what works for our app. Heck, even google's own apps don't obey material design sometimes.

So, what I suggest is keeping the drawer icon and dropping the up button, the drawer can be shorthand for many things like downloads, home, trending, etc. Up button will take you to home, I think drawer gives more functionality. "Home" in drawer still emulates the up button, it's 2 presses away rather than 1. But, it's a good trade given navigation drawer is giving a variety of functionality.

I agree will your decision to stay with your way as almost every android device have back buttons or touch points on them to make the "up" (back) button redundant.

@wb9688
Copy link
Contributor

wb9688 commented Mar 27, 2020

Up is not the same as back. I think it shouldn't show the hamburger, but keep the up button, and then just allow opening the drawer from swiping.

@surrodox2001
Copy link

Up is not the same as back. I think it shouldn't show the hamburger, but keep the up button, and then just allow opening the drawer from swiping.

Seems you've the same conclusions as @mauriciocolli isn't it?

I think the up buttton should be removed and hamburger reintroduced as most android devices have back buttons on them that have the same function as up buttons in app UI.

@opusforlife2
Copy link
Collaborator

Up button will take you to home, I think drawer gives more functionality.

That's the thing. The up button is not 'just another home button' or a duplicate of the back button. It goes one level up in the app screen hierarchy. For example, if you go to the search fragment, then open a search result, tapping the up button takes you one level up to the search fragment, and not the home screen, which needs another tap of the button.

I think the up buttton should be removed and hamburger reintroduced as most android devices have back buttons on them that have the same function as up buttons in app UI.

Another thing to note here is that you can open several further videos after that first one, but the up button will still take you to the search fragment, showing how it's different from the OS back button.

Personally, I don't use it. But I can imagine a situation where someone has been opening several videos in a chain, reached somewhere unrelated, and has forgotten the search term. Having to go back all the way with the OS back button would be irritating, if the up button were not there anymore.

Anyway, at this point, in my head I'm going "Just put both the buttons side by side! I don't care anymore! (╯°□°)╯︵ ┻━┻ " because I really want the drawer.

@surrodox2001
Copy link

Anyway, at this point, in my head I'm going "Just put both the buttons side by side! I don't care
anymore! (╯°□°)╯︵ ┻━┻ " because I really want the drawer.

Best choice really.

@opusforlife2 opusforlife2 added feature request Issue is related to a feature in the app GUI Issue is related to the graphical user interface labels Sep 12, 2020
@Stypox
Copy link
Member

Stypox commented Jan 16, 2021

Now that Unified UI was merged, and the drawer is accessible from the video detail fragment, is this still relevant?

@surrodox2001
Copy link

Now that Unified UI was merged, and the drawer is accessible from the video detail fragment, is this still relevant?

I think this pull is effectively 'closed' because the things has done in an 'another' way, so... (and this' for the old ui anyway, so more irrelevant?)

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
7 participants