1232868 - Clean up bookmark folder list item UI

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Attachment: prev_bookmarks_desktopfolder.png

Our list item UI for folders is dated and doesn't align with the rest of our list items. We should unify them while we're doing all this work so they align to the our guide lines.

I'll post assets and specs.

Comment 1

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

I forgot to mention one important thing. This actually inherits the same styles as our Synced Tabs Panel. 

The only differences are... 

 1) A "folder" icon instead of a "device" icon (still centered in that same space)
 2) Use Roboto light 16sp instead of 14sp (for folder name)

Comment 2

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

Attachment: icon_folder.zip

Attaching assets

Comment 3

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

Was chatting with nalexander on IRC about where we might find these styles:

<@nalexander> we can easily extract the Android styles for the layouts from the code.
<@nalexander> https://dxr.mozilla.org/mozilla-central/search?q=path%3Amobile%2F*.xml+remote&redirect=true&case=false shows some of the styles and XML files.

Comment 4

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

Attachment: icon_backdir.zip

Oh - forgot this! This is the icon for "Back to Bookmarks"

Comment 5

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1232868 - Clean up bookmark folder list item UI. r=mcomella

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

Comment 6

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1232868 - Consolidate usage of arrow resource. r=mcomella

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

Comment 7

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Improved bookmark folder UI

Comment 8

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

(In reply to Chenxia Liu [:liuche] from comment #7)
> Created attachment 8726486 [details]
> Screenshot: Improved bookmark folder UI

Wow. such polish. much happy. many likes!

Let's go with "Back to Desktop Bookmarks" though, it's more precise and requires less thinking (what is "up to"?)

Comment 9

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1232868 - Update strings. r=mcomella

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

Comment 10

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

Comment on attachment 8726831 [details]
MozReview Request: Bug 1232868 - Update strings. r=mcomella

https://reviewboard.mozilla.org/r/38243/#review34849

::: mobile/android/base/locales/en-US/android_strings.dtd:553
(Diff revision 1)
>  <!-- Localization note (home_move_up_to_filter): The variable is replaced by the name of the

Update l10n note

Comment 11

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

Comment on attachment 8726485 [details]
MozReview Request: Bug 1232868 - Consolidate usage of arrow resource. r=mcomella

https://reviewboard.mozilla.org/r/38041/#review34851

Dope.

Comment 12

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

Comment on attachment 8726484 [details]
MozReview Request: Bug 1232868 - Clean up bookmark folder list item UI. r=mcomella

https://reviewboard.mozilla.org/r/38039/#review34853

::: mobile/android/base/java/org/mozilla/gecko/home/TwoLinePageRow.java:94
(Diff revision 1)
>          // Merge layouts lose their padding, so set it dynamically.
> -        setPadding(0, 0, (int) getResources().getDimension(R.dimen.page_row_padding_right), 0);
> +        setPadding(0, 0, (int) getResources().getDimension(R.dimen.page_row_edge_padding), 0);

Just to write this down somewhere, this should be done in a different bug but imo this padding should get set where `TwoLinePageRow` is used in xml, rather than dynamically (and we should probably include the children in the same layout too, probably with `include`, rather than inflating via code. Lucas seemed to like this pattern but I think it's harder to follow)

::: mobile/android/base/resources/drawable/inset_arrow_up.xml:6
(Diff revision 1)
> +<inset xmlns:android="http://schemas.android.com/apk/res/android"

Since inset drawables rely on their parent container's sizes, this should probably be renamed to something indicative of the parent container, e.g. `bookmark_folder_arrow_up`.

nit: Also, add a comment explaining how this works.

::: mobile/android/base/resources/layout/two_line_page_row.xml:14
(Diff revision 1)
> -                                          android:layout_margin="16dp"
> +                                          android:layout_margin="@dimen/page_row_edge_padding"

nit: Why is this dimen shared between this favicon view margin and the page row padding? They don't seem entirely related to me.

::: mobile/android/base/resources/values/styles.xml:166
(Diff revision 1)
>      <style name="Widget.BookmarkFolderView" parent="Widget.TwoLinePageRow.Title">

Now that you removed the style for the other device configuration, this style is only used once, meaning these attrs can be moved out of styles.xml and directly onto the view that the style is used on.

::: mobile/android/base/resources/values/styles.xml:170
(Diff revision 1)
> -        <item name="android:drawablePadding">10dip</item>
> +        <item name="android:paddingLeft">@dimen/page_row_edge_padding</item>
> +        <item name="android:drawablePadding">@dimen/page_row_edge_padding</item>

Same here regarding the dimen.

Comment 13

[Chenxia Liu [:liuche]]

https://reviewboard.mozilla.org/r/38039/#review34853

> nit: Why is this dimen shared between this favicon view margin and the page row padding? They don't seem entirely related to me.

Since the favicon is the leftmost view I used this margin to keep the left "padding" consistent with other places it's used. And since the margin needed to match left and right...I agree that there are lots of style inconsistencies, and perhaps we should have some small discusso about how we want to use the styles throughout. They are kind of messy.

> Now that you removed the style for the other device configuration, this style is only used once, meaning these attrs can be moved out of styles.xml and directly onto the view that the style is used on.

I would prefer to leave this in styles because it is still a coherent collection of attributes and this gives that somewhat eclectic collection some semantic value. If we didn't do this, it is kind of confusing what styles are attributed to the item vs mixed in with other attrs.

> Same here regarding the dimen.

This is still used in a couple of places (and I think it gives semantic meaning to the value) so I'd like to keep it in.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/0ac87c4bee7a
https://hg.mozilla.org/integration/fx-team/rev/3e85c7cca6b7
https://hg.mozilla.org/integration/fx-team/rev/3cc953c09d34

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0ac87c4bee7a
https://hg.mozilla.org/mozilla-central/rev/3e85c7cca6b7
https://hg.mozilla.org/mozilla-central/rev/3cc953c09d34