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

Use material theme on Android 5.0+ #137

Merged
merged 3 commits into from Nov 18, 2015

Conversation

ericwa
Copy link
Contributor

@ericwa ericwa commented Nov 6, 2015

Small change to use the Material theme on Android 5.0+.

I didn't bother copying using ebookdroid.ActionBarStyle in the new theme, because all that seemed to be doing was setting the foreground and background colors to @color/actionbar_white and @color/actionbar_black

We could customize the status bar / action bar colours, see: http://developer.android.com/training/material/theme.html , but I'm not sure what they should be so I just left it alone. This gives a black status bar and dark grey action bar.

With the material theme, the progress spinner when a page is loading is gone now, and the Action Bar icon is gone as well.

Here are some screenshots:
screen1
screen2
screen3

@dschuermann
Copy link
Member

With the material theme, the progress spinner when a page is loading is gone now, and the Action Bar icon is gone as well.

What's the idea to fix this / reintroduce this?

@ericwa
Copy link
Contributor Author

ericwa commented Nov 7, 2015

For the app icon in the toolbar, the Toolbar docs recommend leaving it off:

In modern Android UIs developers should lean more on a visually distinct color scheme for toolbars than on their application icon. The use of application icon plus title as a standard layout is discouraged on API 21 devices and newer.

(from http://developer.android.com/reference/android/widget/Toolbar.html )

Also - if we merge my other PR that adds the "Up" button to the viewer activity, the Up button (with the Material theme) takes up the space that was previously used by the app icon.

For the progress spinner, it sounds like the API's that were being used are no-op with the Material theme. I found some info here: http://stackoverflow.com/questions/27788195/setprogressbarindeterminatevisibilitytrue-not-working
I was able to reintroduce the spinner using the ActionBar.setCustomView() method. I need to tidy it up a bit, and will add it onto this pull request in a bit.

@KrasnayaPloshchad
Copy link
Contributor

Great, I also think the current UI can be modified to use Material design. Futhermore, you can also try to implement the immersive mode (mentioned in #131), which makes system status bar transcluent, this mode should also be activated in fullscreen mode for 5 seconds through scroll, zoom, and just touch the page.

@ericwa
Copy link
Contributor Author

ericwa commented Nov 18, 2015

The progress spinner implementation is working well for me. If we are OK with dropping the app icon from the toolbar, which is the norm for material design, I guess this is ready for review. (sorry don't mean to pester :)

I've also done some more research on the appcompat support library.
We may want to look at adding a depedency on the v7 appcompat and the design support library, and then we could use the new TabLayout class to implement the tabs on the wooden bookshelf, which AFAIK is the current recommended way to do tabs. See: http://www.androidhive.info/2015/09/android-material-design-working-with-tabs/

The appcompat library also gives you the material theme on android versions down to API7, and ActionBar/Toolbar on those devices as well. Not sure if we'd want to use that or not.
Anyway, that could be discussed later / in another pr.

@dschuermann
Copy link
Member

@ericwa Code looks good to me. Would accept 👍

@tuxor1337 are you okay with this?

@tuxor1337
Copy link

Yes, I'm okay with this. By the way, how about giving @ericwa push permissions? He's been doing a really great job here lately. :)

tuxor1337 pushed a commit that referenced this pull request Nov 18, 2015
Use material theme on Android 5.0+
@tuxor1337 tuxor1337 merged commit 1d40d62 into SufficientlySecure:master Nov 18, 2015
@dschuermann
Copy link
Member

@ericwa I added you to the dev group, you should now have write access.

@ericwa
Copy link
Contributor Author

ericwa commented Nov 18, 2015

Awesome, thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants