Issue/drawables with tint (Part 1) #8881

theck13 · 2018-12-28T18:07:29Z

Fix

Update drawable usages with tint so that resources can be consolidated and minimized. Since the minSdkVersion is now 21, we can use the android:tint="color" attribute and ImageView.setImageTintList(ColorStateList) method for all drawables. That allows us combine all drawables with the same path into a single file, which removes redundant files and reduces the app footprint. Using tint also helps us with theming and app-wide color changes in the future.

I chose to make white (#ffffff) the default color for drawables since white is the absence of pigment and that correlates to the absence of tint. It makes differentiating tinted and non-tinted icons easier also.

Note

This is the first installment of the Drawables with Tint series.

Test

Sites

Go to Sites tab.
Notice icons are shown on screen.

Quick Start

Select site with Quick Start in progress.
Go to Sites tab.
Tap Quick Start item.
Tap View your site task.
Notice View Site icon is shown in snackbar.
Tap Quick Start item.
Tap Choose a theme task.
Notice Themes icon is shown in snackbar.
Tap Quick Start item.
Tap Customize your site task.
Notice Themes icon is shown in snackbar.

Stats

Select site with statistics.
Go to Sites tab.
Tap Stats item.
Tap Insights tab.
Tap View More button in Latest Post Summary card.
Notice chevron and other icons are shown in cards.
Tap back arrow action in toolbar.
Notice Posts, Views, Visitors, and Best Views Ever icons are shown in All Time Stats card.
Tap Years tab.
Notice page and post icons are shown in Posts and Pages card.
Notice chevron icons are shown in Authors card.

Site Pages

Select site with at least one page.
Go to Sites tab.
Tap Site Pages under Publish section.
Tap tab with page(s).
Notice ellipsis icon is shown for each page list item.

Plugins

Select site able to install plugins.
Go to Sites tab.
Tap Plugins item under Configuration section.
Tap plugin.
Notice external chevron icons are shown in plugin detail screen.

Sharing

Go to Sites tab.
Tap Sharing item under Configuration section.
Notice cog icon is shown in Sharing Buttons section.

Reader

Select account with at least one site followed.
Go to Reader tab.
Notice ellipsis button is shown on post.
Select Discover in toolbar filter.
Tap post header of unfollowed site.
Notice Follow icon is shown.
Tap X action in toolbar.
Tap ellipsis button on post from site not followed.
Notice Follow, Share, and Block this site icons are shown in menu.
Select Followed Sites in toolbar filter.
Tap post header of followed site.
Tap X action in toolbar.
Tap ellipsis button on post.
Notice Following, Turn on site notifications or Turn off site notifications, Share, and Block this site icons are shown in menu.
Tap Edit tags and sites action in toolbar.
Notice Follow icon is shown in Enter a URL or tag to follow field.

Me

Go to Me tab.
Notice icons are shown on screen.

Notification Settings

Select account with at least one site.
Go to Me tab.
Tap Notification Settings item.
Tap site under Your Sites section.
Notice Notifications tab, Email, and All notifications icons are shown on screen.

Review

Only one developer is required to review these changes, but anyone can perform the review.

… adapter class

…n layout

…e in layout

…tings fragment

…ron image style" This reverts commit d36165e.

...rc/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsFragment.java

hypest · 2019-01-02T14:41:41Z

WordPress/src/main/java/org/wordpress/android/ui/quickstart/QuickStartMySitePrompts.kt

@@ -20,23 +20,23 @@ enum class QuickStartMySitePrompts constructor(
            R.id.my_site_scroll_view_root,
            R.id.row_view_site,
            R.string.quick_start_dialog_view_site_message_short,
-            R.drawable.ic_globe_grey_24dp,
+            R.drawable.ic_globe_white_24dp,


Now that we rely on the tint to have the final color, I find it a little confusing to see the "white" in the drawable name. It feels minor but, I think I'd suggest to completely remove the color mention in the resource name so to avoid "misleading" the reader of the code here. The reader will have to hunt down the setting of the tint to understand what the color will be. WDYT?

I agree. I considered removing the color from the file name as well, but decided to leave it since we will not be able to use tinting for all drawables yet (e.g. the android:drawableTint attribute requires minSdkVersion 23). I thought leaving the color for icon variations would help avoid confusion by being more explicit.

Hmm, but, is ic_globe_white_24dp ever going to be used as white indeed? My understanding is that we will always tint it (maybe via code it is as compat as possible) to some color and so, the "white" in its name will never actually apply. Is my assumption wrong?

The two instances included in these changes, QuickStartMySitePrompts and my_site_fragment, tint the icon from grey to blue_light and grey_darken_30, respectively. There are two other instances, menu/reader_detail and menu/webview, which were using the ic_globe_white_24dp icon before these changes.

Ideally, we can reuse an existing drawable resource and delete an unnecessary drawable resource by replacing ic_globe_grey_24dp with ic_globe_white_24dp. In the case of ic_globe_grey_24dp, it is used by the wpIconDrawable attribute in WPLoginInputRow views, which currently doesn't support tinting. I tried to differentiate tinted and non-tinted icons with white and non-white naming, respectively.

Ah, I see, thanks for the extra context Tyler.

I tried to differentiate tinted and non-tinted icons with white and non-white naming, respectively.

I think that's the subtle detail we'd like to find a good way to convey to the other devs, including our future selves :). I.e. we'd like to encourage the use of the _white_ variant and tint it, unless tinting is cumbersome or unsupported. Any ideas on how to do this encouragement?

Some options that come to mind:

Remove the _white_ specifier to make the name look more general and thus, more "default" to chose or

Add an extra specifier in the name, like _untinted_ or _generic_ or

Investigate whether the linter can help out and warn when _white_ is used untinted

In any case, I think the current PR is OK as is and additional work can be done in follow up PR(s) so, will ✅ this PR.

I agree. I plan to write a post and update the Drawable Resources section in the CONTRIBUTING.md file to help other developers and our future selves with the tinting, naming, etc.

Sounds good to me!

hypest · 2019-01-02T14:59:25Z

WordPress/src/main/java/org/wordpress/android/ui/uploads/PostUploadNotifier.java

@@ -353,7 +353,7 @@ void updateNotificationSuccessForPost(@NonNull PostModel post, @NonNull SiteMode
                                                                            isFirstTimePublish, true, false);
            PendingIntent pendingIntent = PendingIntent.getService(mContext, 0, publishIntent,
                                                                   PendingIntent.FLAG_CANCEL_CURRENT);
-            notificationBuilder.addAction(R.drawable.ic_posts_grey_24dp, mContext.getString(R.string.button_publish),
+            notificationBuilder.addAction(R.drawable.ic_posts_white_24dp, mContext.getString(R.string.button_publish),


Having trouble finding where do we set the tint for this one, any quick pointers @theck13 ?

We don't actually tint these icons in our code. They are automagically tinted by the operating system. You can see the share icon just above this change is white also. Some icons aren't even displayed on certain API versions. See the screenshots below for illustration.

Thanks for the pointer Tyler!

Not a blocker but, do you think we could add a line comment to add this detail? Feel free to if you agree, or leave as is otherwise 👍.

Since the ic_posts_grey_24dp drawable used before these changes didn't actually correspond to the color used by the notification and all the other vector icons in PostUploadNotifier class are white, I don't think a comment is needed.

WordPress/src/main/res/layout/stats_list_cell.xml

hypest

Nice job here @theck13 !

I only left a few comments but none is a blocker. Let me know what you think, thanks!

theck13

Thanks for the review! I believe I replied to all of your comments. Let me know if I missed anything.

WordPress/src/main/res/layout/stats_list_cell.xml

theck13 · 2019-01-02T16:53:44Z

WordPress/src/main/java/org/wordpress/android/ui/quickstart/QuickStartMySitePrompts.kt

@@ -20,23 +20,23 @@ enum class QuickStartMySitePrompts constructor(
            R.id.my_site_scroll_view_root,
            R.id.row_view_site,
            R.string.quick_start_dialog_view_site_message_short,
-            R.drawable.ic_globe_grey_24dp,
+            R.drawable.ic_globe_white_24dp,


I agree. I considered removing the color from the file name as well, but decided to leave it since we will not be able to use tinting for all drawables yet (e.g. the android:drawableTint attribute requires minSdkVersion 23). I thought leaving the color for icon variations would help avoid confusion by being more explicit.

theck13 · 2019-01-02T18:24:26Z

WordPress/src/main/java/org/wordpress/android/ui/uploads/PostUploadNotifier.java

@@ -353,7 +353,7 @@ void updateNotificationSuccessForPost(@NonNull PostModel post, @NonNull SiteMode
                                                                            isFirstTimePublish, true, false);
            PendingIntent pendingIntent = PendingIntent.getService(mContext, 0, publishIntent,
                                                                   PendingIntent.FLAG_CANCEL_CURRENT);
-            notificationBuilder.addAction(R.drawable.ic_posts_grey_24dp, mContext.getString(R.string.button_publish),
+            notificationBuilder.addAction(R.drawable.ic_posts_white_24dp, mContext.getString(R.string.button_publish),


We don't actually tint these icons in our code. They are automagically tinted by the operating system. You can see the share icon just above this change is white also. Some icons aren't even displayed on certain API versions. See the screenshots below for illustration.

...rc/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsFragment.java

hypest

I left a comment in #8881 (comment) but otherwise, LGTM!

theck13

Thanks for the thorough review! I replied to your last comment. If you agree and there is nothing remaining, I think we're good to merge this one.

theck13 · 2019-01-04T13:36:01Z

WordPress/src/main/java/org/wordpress/android/ui/quickstart/QuickStartMySitePrompts.kt

@@ -20,23 +20,23 @@ enum class QuickStartMySitePrompts constructor(
            R.id.my_site_scroll_view_root,
            R.id.row_view_site,
            R.string.quick_start_dialog_view_site_message_short,
-            R.drawable.ic_globe_grey_24dp,
+            R.drawable.ic_globe_white_24dp,


I agree. I plan to write a post and update the Drawable Resources section in the CONTRIBUTING.md file to help other developers and our future selves with the tinting, naming, etc.

theck13 added 30 commits December 26, 2018 20:28

Add image tint to icon of view holder in reader menu adapter class

af76a26

Remove unnecessary conditional for icon image resource in reader menu…

7fba95e

… adapter class

Rename variable to color resource in reader menu adapter class

8be38c2

Add tint to image in reader follow button class

ffd7d57

Add tint attribute to views with reader follow blue medium drawable i…

934d223

…n layout

Update icon reader follow blue medium drawable references to white

0ab31c3

Add tint attribute to views with reader following alert green drawabl…

b9ac163

…e in layout

Update reader popup menu item layout with follow icon and text

6b00daa

Update icon reader following alert green drawable references to white

7ca42b1

Update icon block grey dark drawable references to white

42844c1

Update icon bell alert green drawable references to white

2f9c7be

Update icon bell grey dark drawable references to white

56ecc64

Update icon share grey dark drawable references to white

5fdd80c

Update icon list checkmark grey drawable references to white

79ecf15

Update icon stats alt grey drawable references to white

56e7bd1

Update icon history alt grey drawable references to white

8efcafa

Update icon plans grey drawable references to white

6e9302b

Update icon pages grey drawable references to white

9d81386

Update icon posts grey drawable references to white

13bb2f8

Update icon media grey drawable references to white

c62b39d

Update icon comment grey drawable references to white

348dc3c

Update icon themes grey drawable references to white

b7fe044

Update icon user grey drawable references to white

aaf1d46

Update icon cog grey drawable references to white

745fc0c

Update icon globe grey drawable references to white

45bbc9f

Update icon external black drawable references to white

45a4a15

Update icon help grey drawable references to white

1dc80c4

Update icon sign out grey drawable references to white

14a827a

Update preference icon source to tinted drawable in notifications set…

e922c81

…tings fragment

Update icon bell grey darken drawable references to white

af73fc7

Revert "Add tint attribute to plugin card view secondary element chev…

39efede

…ron image style" This reverts commit d36165e.

theck13 added [Status] Needs Code Review General labels Dec 28, 2018

theck13 added this to the 11.6 milestone Dec 28, 2018

theck13 requested review from oguzkocer, hypest and mzorz December 28, 2018 18:07

hypest reviewed Jan 2, 2019

View reviewed changes

...rc/main/java/org/wordpress/android/ui/prefs/notifications/NotificationsSettingsFragment.java Show resolved Hide resolved

hypest reviewed Jan 2, 2019

View reviewed changes

WordPress/src/main/res/layout/stats_list_cell.xml Show resolved Hide resolved

hypest reviewed Jan 2, 2019

View reviewed changes

theck13 commented Jan 2, 2019

View reviewed changes

hypest approved these changes Jan 4, 2019

View reviewed changes

hypest assigned theck13 Jan 4, 2019

hypest removed the [Status] Needs Code Review label Jan 4, 2019

theck13 commented Jan 4, 2019

View reviewed changes

theck13 assigned hypest and unassigned theck13 Jan 4, 2019

hypest merged commit 248532b into develop Jan 4, 2019

hypest deleted the issue/drawables-with-tint branch January 4, 2019 13:41

theck13 restored the issue/drawables-with-tint branch January 4, 2019 14:18

theck13 mentioned this pull request Jan 5, 2019

Issue/drawables with tint (Part 2) #8900

Merged

This was referenced Feb 7, 2019

Issue/drawables with tint (Part 3) #9193

Merged

Issue/drawables with tint (Part 4) #9205

Merged

Issue/drawables with tint (Part 5) #9213

Merged

Issue/drawables with tint (Part 6) #9218

Merged

Issue/add tint attribute #9223

Merged

theck13 mentioned this pull request Feb 25, 2019

Issue/add color utilities #9328

Merged

theck13 mentioned this pull request Sep 4, 2019

Issue/10433 semantic color names pt2 #10479

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue/drawables with tint (Part 1) #8881

Issue/drawables with tint (Part 1) #8881

theck13 commented Dec 28, 2018 •

edited

Loading

hypest Jan 2, 2019

theck13 Jan 2, 2019

hypest Jan 3, 2019

theck13 Jan 3, 2019

hypest Jan 4, 2019

theck13 Jan 4, 2019

hypest Jan 4, 2019

hypest Jan 2, 2019

theck13 Jan 2, 2019

hypest Jan 3, 2019

theck13 Jan 3, 2019

hypest left a comment

theck13 left a comment

theck13 Jan 2, 2019

theck13 Jan 2, 2019

hypest left a comment

theck13 left a comment

theck13 Jan 4, 2019

Issue/drawables with tint (Part 1) #8881

Issue/drawables with tint (Part 1) #8881

Conversation

theck13 commented Dec 28, 2018 • edited Loading

Fix

Note

Test

Sites

Quick Start

Stats

Site Pages

Plugins

Sharing

Reader

Me

Notification Settings

Review

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hypest left a comment

Choose a reason for hiding this comment

theck13 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hypest left a comment

Choose a reason for hiding this comment

theck13 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theck13 commented Dec 28, 2018 •

edited

Loading