Closed Bug 1194033 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed

People

(Reporter: mcomella, Assigned: zerok60, Mentored)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [good first bug][lang=java])

Attachments

(1 file, 2 obsolete files)

They're currently #33000000 – does toolbar_grey_pressed sound right, antlam?
Flags: needinfo?(alam)
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
Flags: needinfo?(alam)
Summary: Change color of pressed state in list items history and remote tabs to toolbar_grey_pressed → Change color of pressed state in list items history and remote tabs to about_page_header_grey
Assignee: nobody → ally
Assignee: a.m.naaktgeboren → nobody
Mentor: michael.l.comella
Whiteboard: [good first bug][lang=java]
Changed color of pressed for History and Synced Tabs. Should bookmarks follow the same behavior?
Attachment #8701273 - Flags: review?(michael.l.comella)
Assignee: nobody → zerok60
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?
Attachment #8701273 - Flags: review?(michael.l.comella) → feedback+
Added the missing layout file
Attachment #8701273 - Attachment is obsolete: true
Attachment #8702662 - Flags: review?(michael.l.comella)
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.
Attachment #8702662 - Flags: review?(michael.l.comella) → feedback+
Summary: Change color of pressed state in list items history and remote tabs to about_page_header_grey → Change color of pressed state for list items in home panels to about_page_header_grey
Alright. I'll be working on this tonight.
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?
Attachment #8705906 - Flags: review?(michael.l.comella) → feedback+
Any movement here, Angel?
Flags: needinfo?(zerok60)
NI self to land.
Flags: needinfo?(zerok60) → needinfo?(michael.l.comella)
https://hg.mozilla.org/integration/fx-team/rev/aefae03959a33e8e5761e448e7ff6aad0610402e
Bug 1194033 - Added background selector to TwoLinPageRow & removed selector override from RemoteTabsListView r=mcomella
I rebased the patch and landed it. Thanks for your help, Angel!
Flags: needinfo?(michael.l.comella)
https://hg.mozilla.org/mozilla-central/rev/aefae03959a3
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Depends on: 1268603
You need to log in before you can comment on or make changes to this bug.