Closed Bug 1232866 Opened 4 years ago Closed 4 years ago

Establish clearer IA for panel list items

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 47
Tracking Status
firefox47 --- fixed

People

(Reporter: antlam, Assigned: liuche)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

Attachments

(5 files)

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).
Attached image 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)
Attached file icon_bookmarked.zip
Bookmark star - its blue!
Duplicate of this bug: 1164654
(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).
This doesn't block the reading list migration.
No longer blocks: migrate-RL
Attached image Screenshot: New layout
I also verified that the divider is the right color and size.
Attachment #8724280 - Flags: feedback?(alam)
Comment on attachment 8724280 [details]
Screenshot: New layout

uh-MAZING!
Attachment #8724280 - Flags: feedback?(alam) → feedback+
Assignee: nobody → 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 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.
Attachment #8724281 - Flags: review?(michael.l.comella) → review+
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.
> ::: 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.
https://hg.mozilla.org/mozilla-central/rev/a0978b299ee5
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
(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.
Depends on: 1320653
You need to log in before you can comment on or make changes to this bug.