Add ownCloud color to notifications for lollipop #1177

AndyScherzinger · 2015-09-27T19:43:22Z

…for progress bar and icon background.

It is quite a details thing, see screenshot. The color setting on the NotificationCompat.Builder just has an effect on Lollipop devices and up.

Please Review:
Design: @jancborchardt
Code: @davivel @masensio @tobiasKaminsky

…or progress bar and icon background

tobiasKaminsky · 2015-09-30T19:49:55Z

Code looks good 👍

masensio · 2015-10-02T08:24:38Z

👍
Ready to be tested

rperezb · 2015-10-07T08:53:40Z

src/com/owncloud/android/notifications/NotificationBuilderWithProgressBar.java

@@ -55,7 +55,8 @@
        if (Build.VERSION.SDK_INT < Build.VERSION_CODES.ICE_CREAM_SANDWICH) {
            return new NotificationBuilderWithProgressBar(context); 
        } else {
-            return new NotificationCompat.Builder(context);
+            return new NotificationCompat.Builder(context).
+                    setColor(context.getResources().getColor(R.color.primary));


@AndyScherzinger could it be possible to use this color https://github.com/owncloud/android/blob/master/res/values/setup.xml#L34 instead of the color.primary? The reason is because we have some branded apps and we need this change to bit there too.

@jancborchardt @MTRichards what do you think? what Andy has modified is the background color of the notification image, I am suggesting to use the same one that it´s used on the top bar of the app

@AndyScherzinger please, don´t modify till we have their input 😸 thanks!

@rperezb primary is the action bar color :) - with the move to material design the primary color defines the color of the action bar. the color you referred to isn't used anymore in the app. So this is right now legacy code looking at the android app but might still be used by other third party apps who haven't moved to material with the AppCompat implementation yet.

In case we want to define this for third party apps also would mean we would have to move the primary color etc to the setup.xml file

Besides this wouldn't have any impact on branded apps since the notifications are part of the app thus as long as other apps don't set the color exactly the way we would do now in our own code this wouldn't have any impact on their app's notifications :(

rperezb · 2015-10-08T07:45:15Z

ummm, not sure, using this branch. If I modify the color: https://github.com/owncloud/android/blob/master/res/values/setup.xml#L34 the top bar is modified, for instance:

Yes, notifications are part of the app, however, we modify the image used on the notifications so that in some cases other images but the ownCloud regular ones are used. In this branch we are forcing to have the color.primary as background color and sometimes, this is not the expected one, we´d rather to have some kind of flexibility here.
It´d be great if you use, at least, for this concrete option, the top bar color variable.

thanks!

AndyScherzinger · 2015-10-08T14:20:38Z

Argh, now I found the issue.... this depends on the android version (could consider it a bug in my implementation). The actionbar uses the color-id you mentioned anything else doesn't (as in uses the primary color).

So my suggestion is: I will "map" the primary color (material design id) to the color-id you mentioned above and from there simply work with the primary, so whenever you change the color you mentioned above it will also work throughout the other definitions :)

rperezb · 2015-10-08T16:30:02Z

sold! I talked offline with @MTRichards and he agrees in that color as a based one too :)

Great team work! (as usual)

MTRichards · 2015-10-08T18:26:21Z

Thanks @AndyScherzinger and @rperezb

…ification_coloring

…olor to enable proper color manipulation. Technical debt: setup should rename the actionbar_start_color (and actionbar_end_color) since it is basically the primary-oiiod value due to material design terms.

AndyScherzinger · 2015-10-09T16:21:20Z

@rperezb @MTRichards I committed the change thus you should now be able to change the color which would also have an impact on the notification color (completely untested atm).
Now here's the BUT; the material design defines some more colors/ids which aren't available in the setup.xml:

you will only be able to change the main bar's color (as shown on your screenshot)
you cannot change the system bars color
you cannot change the secondary button color (pattern is part of Material buttons and checkboxes #1090)
you cannot change the check box color (part of Material buttons and checkboxes #1090) since they are image based
you cannot change the progress indicator below the action bar they it is also image based + animation

So my proposal would now be:

keep and merge this PR as it is now
@rperezb opens a new issue which defines the things that you want to be able to have control over using screenshots hinting which elements you need color control for
assign the ticket to me
I will copy the necessary color-ids into your setup file so you are able to manipulate all the defined elements accordingly :)
last but not least, can you @rperezb clarify if and how the ids would have to be/stay in the setup.xml, just asking since the start/end color naming pattern for the actionbar doesn't make sense for material themend apps. My proposal here would be to keep them for now to be backwards compatible for third party apps, have the primary next to it and remove the lecagy ones at some point in time. :)

Any comments on this approach @rperezb @MTRichards ?

MTRichards · 2015-10-09T17:54:39Z

A great teamwork I think.

Matt Richards
VP Products & Markets
ownCloud, inc.

On Oct 9, 2015, at 12:25 PM, Andy Scherzinger [email protected] wrote:

@rperezb @MTRichards I comitted the change thus you should now be able to change the color which would also have an impact on the notification color (completely untested atm).
Now here's the BUT the material design defines some more colors/ids which aren't available in the setup.xml:

you will only be able to change the main bars color (as shown on your screenshot)
you cannot change the system bars color
you cannot change the secondary button color (pattern is part of #1090)
you cannot change the checkbox color (part of #1090) since they are image based
you cannot change the progress indicator below the actionbar they it is also image based + animation
So my proposal would now be:

keep and merge this PR as it is now
@rperezb opens a new issue which defines the things that you want to be able to have control over using screenshots hinting which elements you need color control for
assign the ticket to me
I will copy the neccesary color-ids into your setup file so you are able to manipulate all the defined elemtents accordingly :)
last but not least, can you @rperezb clarify if and how the ids would have to be/stay in the setup.xml, just asking since the start/end color naming pattern for the actionbar doesn't make sense for material themend apps. My proposal here would be to keep them for now to be backwards compatible for third party apps, have the primary next to it and remove the lecagy ones at some point in time. :)
Any comments on this approach @rperezb @MTRichards ?

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

jancborchardt · 2015-10-11T09:42:22Z

Nice! Just one tiny detail: The cloud is not centered visually in the circle. It needs to be moved a bit up and to the right – just a few pixels.

AndyScherzinger · 2015-10-11T10:20:15Z

@jancborchardt I can't fix that ;) - The notification is a Android standard one so the positioning of the cloud depends on the positioning of the "cloud" in the png file itself. The icon within the notification is also the one used by android to be displayed in the system bar.

rperezb · 2015-10-13T11:10:35Z

Thanks a lot @AndyScherzinger
tested and it works just fine:

Besides, in this branch you have modified the color of the text on the login screen:

Which it´s perfect! it was something that we wanted to do 💃

Then, I'll check your comment, #1177 (comment)

@masensio @davivel please review

AndyScherzinger · 2015-10-13T11:32:52Z

Glad I could help. The login screen change is by definition. The text color is based on the colored primary. So as soon as you merge this branch and #1090 this will also impact the primary action button :)

jancborchardt · 2015-10-13T21:46:26Z

Keep in mind there’s a difference between background color and primary action button color though. :)

#1d2d44 (dark blue): header bar, background for this icon, etc.
#35537a (lighter dark blue): primary action button (upload etc), probably heading text in settings (right?), and probably also sidebar part of where the username is shown

Input field text should not really be colored, that looks kind of strange, no?

AndyScherzinger · 2015-10-13T22:36:20Z

Yes, you are right. :)
Input text is also defined by one of these colors by material default color assignment. Would have to check which one, but I think the dark blue is also set for the text field text color.

jancborchardt · 2015-10-13T22:51:50Z

Shouldn’t the text field color just be default (black or off-black I guess)? Otherwise it’s going to look strange for some brandings I guess, especially when they have a light or neon-like header color (which I don’t recommend in the first place, but well).

…ification_coloring

AndyScherzinger · 2015-10-20T21:46:52Z

So how to move on with this PR?

jancborchardt · 2015-10-21T00:39:42Z

As soon as the input field text is fixed, it’s good to go from the design side. : )

…loginscreen

AndyScherzinger · 2015-10-21T09:36:51Z

@rperezb @Dianafg76 I changed the input field coloring according to the @jancborchardt's feedback. Sorry for taking that away from you.

In case this is something you need in a different way and discussed with @jancborchardt I can implement it, but in a different/new PR. Since bloating the PRs lowers the change to get the PR merged and also raises the time it usually takes to get it merged by the team.

rperezb · 2015-10-21T15:31:39Z

Checked it.

The text letters are the default ones, black
@davivel @masensio can you please merge?

davivel · 2015-10-22T07:10:07Z

Code ok 👍

Thanks, @AndyScherzinger

Add ownCloud color to notifications for lollipop

AndyScherzinger · 2015-10-22T07:23:19Z

You are welcome, always a pleasure @davivel :)

Add ownCloud color to notifications (only works on lollipop and up) f…

796b1bd

…or progress bar and icon background

AndyScherzinger mentioned this pull request Sep 27, 2015

Material FAB with speed dial implementation #1100

Merged

AndyScherzinger added enhancement Design labels Sep 28, 2015

masensio added the ReadyToTest label Oct 2, 2015

rperezb added this to the 1.8.1-current milestone Oct 7, 2015

rperezb assigned Dianafg76 Oct 7, 2015

rperezb reviewed Oct 7, 2015
View reviewed changes

rperezb added the Needs info label Oct 7, 2015

AndyScherzinger added 3 commits October 9, 2015 14:45

Merge branch 'master' of https://github.com/owncloud/android into not…

5db8c47

…ification_coloring

directly link owncloud_blue color ID to setup.xml's actionbar_start_c…

a61f862

…olor to enable proper color manipulation. Technical debt: setup should rename the actionbar_start_color (and actionbar_end_color) since it is basically the primary-oiiod value due to material design terms.

actual lib reference

4b4c323

rperezb added Approved by qa and removed Needs info ReadyToTest labels Oct 13, 2015

Merge branch 'master' of https://github.com/owncloud/android into not…

c56b356

…ification_coloring

removed primary color usage for textColor of the input fields on the …

dffd81e

…loginscreen

davivel added a commit that referenced this pull request Oct 22, 2015

Merge pull request #1177 from owncloud/notification_coloring

08443f9

Add ownCloud color to notifications for lollipop

davivel merged commit 08443f9 into master Oct 22, 2015

davivel deleted the notification_coloring branch October 22, 2015 07:10

davivel added the 5 - To release label Nov 12, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ownCloud color to notifications for lollipop #1177

Add ownCloud color to notifications for lollipop #1177

AndyScherzinger commented Sep 27, 2015

tobiasKaminsky commented Sep 30, 2015

masensio commented Oct 2, 2015

rperezb Oct 7, 2015

AndyScherzinger Oct 7, 2015

rperezb commented Oct 8, 2015

AndyScherzinger commented Oct 8, 2015

rperezb commented Oct 8, 2015

MTRichards commented Oct 8, 2015

AndyScherzinger commented Oct 9, 2015

MTRichards commented Oct 9, 2015

jancborchardt commented Oct 11, 2015

AndyScherzinger commented Oct 11, 2015

rperezb commented Oct 13, 2015

AndyScherzinger commented Oct 13, 2015

jancborchardt commented Oct 13, 2015

AndyScherzinger commented Oct 13, 2015

jancborchardt commented Oct 13, 2015

AndyScherzinger commented Oct 20, 2015

jancborchardt commented Oct 21, 2015

AndyScherzinger commented Oct 21, 2015

rperezb commented Oct 21, 2015

davivel commented Oct 22, 2015

AndyScherzinger commented Oct 22, 2015

Add ownCloud color to notifications for lollipop #1177

Add ownCloud color to notifications for lollipop #1177

Conversation

AndyScherzinger commented Sep 27, 2015

tobiasKaminsky commented Sep 30, 2015

masensio commented Oct 2, 2015

rperezb Oct 7, 2015

Choose a reason for hiding this comment

AndyScherzinger Oct 7, 2015

Choose a reason for hiding this comment

rperezb commented Oct 8, 2015

AndyScherzinger commented Oct 8, 2015

rperezb commented Oct 8, 2015

MTRichards commented Oct 8, 2015

AndyScherzinger commented Oct 9, 2015

MTRichards commented Oct 9, 2015

jancborchardt commented Oct 11, 2015

AndyScherzinger commented Oct 11, 2015

rperezb commented Oct 13, 2015

AndyScherzinger commented Oct 13, 2015

jancborchardt commented Oct 13, 2015

AndyScherzinger commented Oct 13, 2015

jancborchardt commented Oct 13, 2015

AndyScherzinger commented Oct 20, 2015

jancborchardt commented Oct 21, 2015

AndyScherzinger commented Oct 21, 2015

rperezb commented Oct 21, 2015

davivel commented Oct 22, 2015

AndyScherzinger commented Oct 22, 2015