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

A bit material design #654

Merged
merged 5 commits into from
Dec 6, 2015
Merged

Conversation

larsgrefer
Copy link
Collaborator

No description provided.

@SecUpwN
Copy link
Member

SecUpwN commented Dec 4, 2015

Huge thanks for this! The update on the Drawer Toggle is causing Travis CI to fail, please see the logfile.

@larsgrefer
Copy link
Collaborator Author

@SecUpwN There seems to be an error with the AVD on travis. I'll have a look at it later

@SecUpwN
Copy link
Member

SecUpwN commented Dec 4, 2015

There seems to be an error with the AVD on travis. I'll have a look at it later

Thanks, I'll be wating with merging until a fix has been added here.

If we wan't to inform about a status change, we should use a "peeking notification":
https://www.google.com/design/spec/patterns/notifications.html#notifications-peeking-notifications

Since we likely cannot circumvent that our app gets displayed incorrectly some day when Google decides to changes more stuff and guidelines of the AOS (don't ask how much we hate that), we've been re-thinking the Icons in the navigation drawer and would be happy if you'd chime in on #512 to solve this.

@agilob
Copy link
Contributor

agilob commented Dec 4, 2015

@SecUpwN you linked to peeking notification, but app uses ongoing notification now. Is anyone planning removing ongoing notification or am I missing something?

@@ -99,11 +99,10 @@ android {
dependencies {
// DO NOT REMOVE BELOW COMMENTED-OUT CODE BEFORE ASKING!
//compile 'com.github.amlcurran.showcaseview:library:5.0.0'
//compile 'com.android.support:appcompat-v7:22.1.1'
compile 'com.android.support:appcompat-v7:22.+'
Copy link
Contributor

Choose a reason for hiding this comment

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

this can cause unexpected behaviour depending on build machine. Please specify version.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

I found it out really hard way...

@larsgrefer
Copy link
Collaborator Author

@agilob @SecUpwN I think there should be one persistent notification with low priority (in order to keep the service alive) and an additional peeking notification with high priority on status changes.

I like this Idea: #512 (comment)

@SecUpwN
Copy link
Member

SecUpwN commented Dec 4, 2015

@larsgrefer, the idea from #512 (comment) is not final yet, let's discuss the usage of notifications there. Current priority on this PR is to solve the following things (thanks ahead for digging deeper):

@larsgrefer
Copy link
Collaborator Author

@SecUpwN #479 was already fixed in #527, was'nt it?

@larsgrefer
Copy link
Collaborator Author

@SecUpwN what is tested by testGetPropsReturnsValue()?
This one fails

@agilob
Copy link
Contributor

agilob commented Dec 4, 2015

LOL
Feel free to remove this "test" (whole file) it tries to start an actual activity on Android, it fails because there is no device with screen on attached to gradle test.

@SecUpwN
Copy link
Member

SecUpwN commented Dec 4, 2015

@SecUpwN #479 was already fixed in #527, was'nt it?

@larsgrefer, partially. The main fix to show the colored notification was deb50bf, which will likely be removed when you update the targetSdkVersion from 19 to 22. Hence my previous question.

Coming back to the failing Travis CI check, please see if you can fix this part of the PR:

104: FAILURE: Build failed with an exception.
105:
106: * Where:
107: Build file '/workspace/source/app/build.gradle' line: 3
108:
109: * What went wrong:
110: A problem occurred evaluating project ':app'.
111: > Cannot run program "git" (in directory "/workspace/source"): error=2, No such file or directory

Feel free to remove this "test" (whole file) it tries to start an actual activity on Android, it fails because there is no device with screen on attached to gradle test.

@agilob, don't we need that testGetPropsReturnsValue() in the future for something like #173?

@larsgrefer
Copy link
Collaborator Author

#656 should fix the travis errors

@SecUpwN
Copy link
Member

SecUpwN commented Dec 5, 2015

Thanks, that indeed fixed the Travis CI check. @larsgrefer, last question regarding this PR: Will #479 come back when we merge this? The purpose was to keep the notification icon colored, I expect that with changing the targetSdkVersion we will likely have a white Icon up there which does not change color accordingly, which in turn makes our Status Icons pretty much useless. Thanks for your endurance.

@larsgrefer
Copy link
Collaborator Author

If we merge this the statusbar-icon will be just a white shape like here: larsgrefer@5ac2763#commitcomment-14785284

But the icon of the full notification will be colored like here: #479 (comment)

I don't think we should stay on pre-lollipop and block material design just to keep a colored status bar icon

In order to inform about a status change i'd suggest to use a "peeking notification":
https://www.google.com/design/spec/patterns/notifications.html#notifications-peeking-notifications

@larsgrefer larsgrefer self-assigned this Dec 5, 2015
@SecUpwN
Copy link
Member

SecUpwN commented Dec 5, 2015

I don't think we should stay on pre-lollipop and block material design just to keep a colored status bar icon

Makes sense, but the reason I previously opened #479 was because users would always have to pull down the notification bar in order to see the current state of our our app (as long as we have not implemented peeking notifications). @larsgrefer, please add the link and your thaoughts about peeking notifications to #512. Regarding the merge of this PR, I would like to hear the opinion of @He3556 as well.

@larsgrefer
Copy link
Collaborator Author

@SecUpwN Good news: with compileSdk 22, appcompat 22 and targetSdk 19 we can have colored statusbar-icons AND material design, so #479 won't come back

@larsgrefer larsgrefer changed the title API level 22 and a bit material design A bit material design Dec 6, 2015
SecUpwN added a commit that referenced this pull request Dec 6, 2015
@SecUpwN SecUpwN merged commit 8324f0b into CellularPrivacy:development Dec 6, 2015
@smarek smarek added this to the v0.1.37-alpha milestone Dec 7, 2015
@larsgrefer
Copy link
Collaborator Author

In fact there is a running avd in an emulator
Am 04.12.2015 10:50 nachm. schrieb agilob notifications@github.com:LOL
Feel free to remove this "test" it tries to start an actual activity on Android, it fails because there is no device with screen on attached to gradle test.

—Reply to this email directly or view it on GitHub.

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

Successfully merging this pull request may close these issues.

None yet

4 participants