1194033 - Change color of pressed state for list items in home panels to about_page_header_grey

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

They're currently #33000000 – does toolbar_grey_pressed sound right, antlam?

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

They're defined in action_bar_button, fwiw: https://mxr.mozilla.org/mozilla-central/search?string=action_bar_button[^_]&regexp=on&find=mobile%2Fandroid&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Edit cancel too.

Comment 3

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Is this for the Home panels? If so, since it's on "White" this color should actually be our "About page header grey" which is #F5F5F5

Comment 4

[Angel Bouzo]

Attachment: Bug 1194033 - Change color of pressed state in list items history and remote tabs to about_page_header_grey r=mcomella.txt

Changed color of pressed for History and Synced Tabs. Should bookmarks follow the same behavior?

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8701273 [details] [diff] [review]
Bug 1194033 - Change color of pressed state in list items history and remote tabs to about_page_header_grey r=mcomella.txt

Review of attachment 8701273 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch!

This looks correct besides the missing file.

::: mobile/android/base/java/org/mozilla/gecko/home/HistoryHeaderListCursorAdapter.java
@@ +21,5 @@
>      private static final int ROW_HEADER = 0;
>      private static final int ROW_STANDARD = 1;
>  
>      private static final int[] VIEW_TYPES = new int[] { ROW_STANDARD, ROW_HEADER };
> +    private static final int[] LAYOUT_TYPES = new int[] { R.layout.home_history_tab_item, R.layout.home_header_row };

It looks like you didn't `hg add home_history_tab_item.xml` – can you do that, refresh the patch, and reupload?

Comment 6

[Angel Bouzo]

Attachment: Bug 1194033 - Change color of pressed state in list items history and remote tabs to about_page_header_grey r=mcomella.txt

Added the missing layout file

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8702662 [details] [diff] [review]
Bug 1194033 - Change color of pressed state in list items history and remote tabs to about_page_header_grey r=mcomella.txt

Review of attachment 8702662 [details] [diff] [review]:
-----------------------------------------------------------------

Actually, looking at this now, I think we should apply this to all of the home panels that use a list rather than just the two listed here. That way, I think you can put your changes into home_item_row.

Sorry for the confusion.

::: mobile/android/services/src/main/res/layout/home_history_tab_item.xml
@@ +3,5 @@
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +<org.mozilla.gecko.home.TwoLinePageRow xmlns:android="http://schemas.android.com/apk/res/android"
> +                                       style="@style/Widget.HistoryItemView"

In general, if a style is not re-used, it's best to leave the attributes in the layout file so that devs don't need to do unnecessary lookups into styles.xml and look at confusing inheritance hierarchies.

Comment 8

[Angel Bouzo]

Alright. I'll be working on this tonight.

Comment 9

[Angel Bouzo]

Attachment: Bug 1194033 - Change color of pressed state for list items in home panels to about_page_header_grey. r=mcomella.txt

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8705906 [details] [diff] [review]
Bug 1194033 - Change color of pressed state for list items in home panels to about_page_header_grey. r=mcomella.txt

Review of attachment 8705906 [details] [diff] [review]:
-----------------------------------------------------------------

I apologize for the delay, Angel – I'm caught up in some other work so you can also flag :ahunt for reviews to make sure your reviews get done in a timely manner.

This does not appear to work in a few panels: the Top Sites grid in the Top Sites panel, reading list, and Synced Tabs. Can you make those work as well?

Also, your patch didn't apply cleanly (though I was able to fix it locally) – can you make sure you pull down the latest revision and rebase your patch?

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Any movement here, Angel?

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

NI self to land.

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/aefae03959a33e8e5761e448e7ff6aad0610402e
Bug 1194033 - Added background selector to TwoLinPageRow & removed selector override from RemoteTabsListView r=mcomella

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I rebased the patch and landed it. Thanks for your help, Angel!

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/aefae03959a3