1232866 - Establish clearer IA for panel list items

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Attachment: Screen Shot 2015-12-15 at 5.27.00 PM.png

I'd like to clean up our home panel list items layouts. This will help establish a clearer information architecture and teach our users how to visually identify and recall the value behind these links consistently.

Attaching preview of how it would look in History, and Synced tabs. Note: the favicons are a part of another bug (bug 1228680).

Comment 1

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

Attachment: spec_listitem.png

Attaching spec.

Ideally, I'm hoping we could use this for most/all of our list items. That way, we can keep this fundamental UI element of "a link" consistent throughout our product.

Broken down, it looks something like this:

The space on the left becomes - "Where is this link going to send me?"
The space to the left of the link underneath becomes - "What type of URL is this?" (RV, open on another device, etc)
The right-most side becomes - "What's the status of this link?" (for now, it is mostly reserved for Bookmark)

Comment 2

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

Attachment: icon_bookmarked.zip

Bookmark star - its blue!

Comment 4

[Michael Comella (:mcomella) [needinfo or I won't see it]]

(In reply to Anthony Lam (:antlam) from comment #1)
> Ideally, I'm hoping we could use this for most/all of our list items. That
> way, we can keep this fundamental UI element of "a link" consistent
> throughout our product.

I wonder if we should create new naming conventions or have a list of our "main" styles somewhere, to make finding and using these styles easier. Perhaps we should name the styles w/ antlam so he can tell us which to use by name, like we do with colors (e.g. fennec_ui_orange).

Comment 5

[:Margaret Leibovic]

This doesn't block the reading list migration.

Comment 6

[Chenxia Liu [:liuche]]

Attachment: Screenshot: New layout

I also verified that the divider is the right color and size.

Comment 7

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1232866 - Establish clearer IA for panel list items. r=mcomella

Review commit: https://reviewboard.mozilla.org/r/36959/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36959/

Comment 8

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

Comment on attachment 8724280 [details]
Screenshot: New layout

uh-MAZING!

Comment 9

[Chenxia Liu [:liuche]]

I also see that we're using similar assets in bug 1227120, but bigger - maybe we should use the same assets between these two bugs.

Comment 10

[Michael Comella (:mcomella) [needinfo or I won't see it]]

Comment on attachment 8724281 [details]
MozReview Request: Bug 1232866 - Establish clearer IA for panel list items. r=mcomella

https://reviewboard.mozilla.org/r/36959/#review33871

r+ w/ a few clean-ups.

::: mobile/android/base/java/org/mozilla/gecko/home/TwoLinePageRow.java:10
(Diff revision 1)
> +import android.widget.ImageView;

nit: I'd complain about import order but I've stopped caring (and wasting time on it) myself. Your call. :)

::: mobile/android/base/java/org/mozilla/gecko/home/TwoLinePageRow.java:94
(Diff revision 1)
> +        setPadding(0, 0, (int) getResources().getDimension(R.dimen.page_row_padding_right), 0);

Why not declare this in the places `TwoLinePageRow` is used in xml (it's corresponding style).

::: mobile/android/base/java/org/mozilla/gecko/home/TwoLinePageRow.java:181
(Diff revision 1)
> -    private void setPageTypeIcon(int iconId) {
> +    private void showPageTypeIcon(boolean toShow) {

This is no longer a `PageTypeIcon` but rather `BookmarkedIcon`, or similar. Can we rename the method here, as well as the view id?

::: mobile/android/base/java/org/mozilla/gecko/home/TwoLinePageRow.java:232
(Diff revision 1)
>              if (bookmarkId == 0) {

nit: `showPageTypeIcon(bookmarkId != 0)`

::: mobile/android/base/resources/layout/two_line_page_row.xml:29
(Diff revision 1)
> -                gecko:fadeWidth="30dp"
> +                gecko:fadeWidth="90dp"

Why was this increased to 90dp? It seems extreme.

::: mobile/android/base/resources/layout/two_line_page_row.xml:33
(Diff revision 1)
>                    style="@style/Widget.TwoLinePageRow.Url"

If you're changing this to the `Faded...TextView`, you've got to remove the ellipsis attrs from the `TwoLinePageRow.Url` style.

::: mobile/android/base/resources/layout/two_line_page_row.xml:43
(Diff revision 1)
> +    <ImageView android:id="@+id/status_icon"

nit: ws above & below

::: mobile/android/base/resources/layout/two_line_page_row.xml:44
(Diff revision 1)
> +               android:layout_width="@dimen/home_status_icon_size"

nit: Any reason not to declare the dp right in the xml here? imo, it's generally better to write the values directly in the file if they're neither re-used nor change via different configuration.

Comment 11

[Chenxia Liu [:liuche]]

https://reviewboard.mozilla.org/r/36959/#review33871

> Why was this increased to 90dp? It seems extreme.

Anthony and I tried a few values in person and decided on this one.

Comment 12

[Chenxia Liu [:liuche]]

https://reviewboard.mozilla.org/r/36959/#review34189

Comment 13

[Chenxia Liu [:liuche]]

> ::: mobile/android/base/java/org/mozilla/gecko/home/TwoLinePageRow.java:94
> (Diff revision 1)
> > +        setPadding(0, 0, (int) getResources().getDimension(R.dimen.page_row_padding_right), 0);
> 
> Why not declare this in the places `TwoLinePageRow` is used in xml (it's
> corresponding style).

A merge layout gets its top-level layout params stripped so you can't declare it in the xml. In this case you also can't declare it in the parent because the parent is a ListView and the params would apply to the whole list, not each list item.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/a0978b299ee5

Comment 15

[Carsten Book [:Tomcat]]

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

Comment 16

[Jan Henning [:JanH]]

(In reply to Chenxia Liu [:liuche] from comment #11)
> https://reviewboard.mozilla.org/r/36959/#review33871
> 
> > Why was this increased to 90dp? It seems extreme.
> 
> Anthony and I tried a few values in person and decided on this one.

I echo that - with the bookmark star in place it sort of works, but for all other tabs it feels like wasted space to fade out so early - I'd rather have a few more title/URL characters visible.