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

Account switching in navigation drawer #1537

Closed
wants to merge 1 commit into from

Conversation

tobiasKaminsky
Copy link
Contributor

As wished in #1383

@tobiasKaminsky
Copy link
Contributor Author

account

Ugly as hell...
@AndyScherzinger can you help me here with this? Pleeeease :) :)

@przybylski
Copy link
Member

I don't know if you want to add more dependencies but you migh wanna check this out https://github.com/mikepenz/MaterialDrawer

@tobiasKaminsky
Copy link
Contributor Author

Seems to be a very good lib, what do you think @AndyScherzinger @jancborchardt @davivel

@AndyScherzinger
Copy link
Contributor

@tobiasKaminsky I'll take a look and pimp it :) I also stumbled across the lib but with the more recent additions to the AppCompat Drawer implementation I am not sure it is worth the dependency. I'll see if we can add the functionality with the AppCompat lib. The neat thing about the Lib is everything comes out of the box (sticky footer, vector support, account switcher) on the other hand it also relies n the AppCompat lib and thus we would then always stay in sync with the versions imho which might be paintful in case we need a bug fix release from that lib which forces an AppCompat update.
I'll see what I can do without the library as a first try :)

@jancborchardt
Copy link
Member

Yep, it should definitely look like the switcher in the linked library. :)

Also, the last entry of the dropdown should then be »Add account« so we can completely remove this section from the settings.

@AndyScherzinger
Copy link
Contributor

In that case we probably should utilize the library with the mentioned benefits like:

  • account switcher
  • vector icons and thus coloring :)

Downside might be that besides a certain AppCompat lock (atm 23.2.1) due to the lib is that it adds some more AppCompat libs and lib internal stuff, see: https://github.com/mikepenz/MaterialDrawer/blob/develop/library/build.gradle

@davivel what is your opinion?

@tobiasKaminsky
Copy link
Contributor Author

Also, the last entry of the dropdown should then be »Add account« so we can completely remove this section from the settings.

I remember that we discussed this and said that the drawer is just a switch for the accounts and should not function as add/remove. This would also keep the drawer clean and simple and focus on the tasks that are often used.

@AndyScherzinger
Copy link
Contributor

The account switcher would automatically bring such a feature, b/c the user switches swaps the list and the below menu/actions so this would be an option to use if we make use of the lib. Else we should keep it simple since the fully blown implementation like the one offered needs a lot of effort to implement by yourself.
Like I said the AppCompat dependency might be an issue. This would be fine with me in case we:

  • wait until after the 1.9.2 release
  • bump the AppCompat libs dependendy accordingly
  • add the drawer lib

@AndyScherzinger
Copy link
Contributor

After a couple of hours of playing around with the actual implementation I couldn't get anywhere style-wise yet 😞

@AndyScherzinger
Copy link
Contributor

I'm still working on it moving on to the actual NavigationView implementation, but might have to have a diskussion with @jancborchardt since that means some (auto) style changes since we will then be using Google's implementation for the drawer menu which has some implications (font, font-weight, etc.). Will keep you posted. In case Jan is fine with that I will also add the account switcher in the way it is known from other material designed apps 😄
It might still be tricky though...

@AndyScherzinger
Copy link
Contributor

Screenshots of the drawer using the Google Support lib can now be found here: #1552 (comment)

@tobiasKaminsky
Copy link
Contributor Author

Just to understand: which lib do you mean?

@AndyScherzinger
Copy link
Contributor

As for the dependency discussion I am referring to https://github.com/mikepenz/MaterialDrawer as for the latest comments by me I am referring to the support lib from Google/Android. As for the https://github.com/mikepenz/MaterialDrawer lib we do get a fully implemented, abstract drawer lib at the cost of a competing appcompat lib dependency which makes things complicated. As for the later that means a bit more work now but then we are "just" depending on the google compatibility libs we are already depending on to support material design all the way back to 4.0

@tobiasKaminsky
Copy link
Contributor Author

Ok, I understand.
Is there a big (visual) difference between the screenshots in #1552 and the current beta version?

I would also rather keep the deps small and implement the account switching by ourselves.
You said, that you tried a couple of hours to design the account switching.
Where is the problem? Maybe I can help you..?

@AndyScherzinger
Copy link
Contributor

The problem I had was with the display of the accounts in the list especially with their size, disctances between radio button icon and circle-letter icon. Besides that I will anyways try to implement it the "standard" way if one can call it that as it is shown in many apps flipping the menu item/list for a accounts list via the account displayed in the drawer's header.

Question is if @jancborchardt is fine with the then changed visuals and if the overall decision trying to implement it ourselfs is also fine with @davivel (which I guess so, since we would not use a lib for that :). So right now I am simply waiting for Jan's comment on this :)

The visual difference between the actual implementation and the Google one is font size and weight...
See comparison Google standard (left) and our actual custom implementation (right)
drawer

@jancborchardt
Copy link
Member

Looks good to me. Our implementation actually has nicer whitespace and the name text properly left aligned with the other list item text. :)

@AndyScherzinger
Copy link
Contributor

So moving to the "new" implementation would be fine? The alignment "only" matters until we move to the toolbar instead of the actionbar which I would have done already but had an unsolvable problem with handling the icon state for drawer/up/back so with 1.8 I simply went back to the (old) action bar. Whenever we move to the actionbar which I would strongly suggest we do then the drawer will hover above the bar and thus the alignment will be fine. Also using the toolbar has some more benefits like customizing and animation/hiding. But in order to do that I might then be in need of some support from @davivel since to me it seemed some event propagation during folder navigation messed of my toolbar icon state.

@AndyScherzinger
Copy link
Contributor

@ALL please see #1559 which is in development introducing a completely new drawer implementation also incorporating a material style account switcher implementation.

<!--android:layout_height="2dip"-->
<!--android:background="@color/list_item_lastmod_and_filesize_text" />-->
<TextView
android:layout_width="fill_parent"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, as a general rule, use "match_parent"; "fill_parent" is obsolete.

@davivel
Copy link
Contributor

davivel commented Apr 19, 2016

@tobiasKaminsky , I made some comments just in case.

@AndyScherzinger , is this included in #1559, or makes it obsolete?

@AndyScherzinger
Copy link
Contributor

#1559 renders this PR obsolete, but we should keep it until 1559 is merged and out in the wild (already in beta since end of last week)

@davivel
Copy link
Contributor

davivel commented Apr 20, 2016

Sorry, I don't understand. Why should be keep this open if it's obsolete?

@AndyScherzinger
Copy link
Contributor

We can close it but I would like to keep the branch itself for now just in case I need to peek for some of Tobias' code. This shouldn't happen, but you never know.

@davivel
Copy link
Contributor

davivel commented Apr 20, 2016

Oh, that's no problem.

Closing as subsumed/merged by #1559. Thanks, @tobiasKaminsky & @AndyScherzinger

@davivel davivel closed this Apr 20, 2016
@davigonz davigonz deleted the accountSwitching branch March 19, 2019 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants