Closed Bug 1246243 Opened 4 years ago Closed 4 years ago

Indicate that the "Reading List" folder is different

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 48
Tracking Status
firefox48 --- verified

People

(Reporter: antlam, Assigned: ahunt)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

Attached image prev_bookmarks_root_migrate6.png (obsolete) —
Should be the same asset from bug 1234328, same padding of 15dp from the right, vertically centered.
Summary: Add "offline" icon to privileged "Reading List" folder → Indicate that the "Reading List" folder is different
Attachment #8716422 - Attachment is obsolete: true
We can use the Reader View icon here as an indicator that it's different to a traditional "folder" but also that it's related to Reader View.
Attached file icon_RLfolder.zip
Optimized these for use in the folder list item.
Depends on: 1257345
This approach is extensible and would allow easy addition of special icons for e.g. the
screenshots folder.

Review commit: https://reviewboard.mozilla.org/r/45855/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45855/
Attachment #8740655 - Flags: review?(liuche)
Assignee: nobody → ahunt
Status: NEW → ASSIGNED
Comment on attachment 8740655 [details]
MozReview Request: Bug 1246243 - Use special icon for reading list folder r?liuche

https://reviewboard.mozilla.org/r/45855/#review43445

r+ with some naming and comment updates.

::: mobile/android/base/java/org/mozilla/gecko/home/BookmarkFolderView.java:20
(Diff revision 1)
>  public class BookmarkFolderView extends TextView {
> -    private static final int[] STATE_OPEN = { R.attr.state_open };
> +    public enum FolderState {
> +        /**
> +         * A standard folder, i.e. a folder in a list of bookmarks and folders.
> +         */
> +        FOLDER(0),

Let's rename these so they're a little more explicit. What do you think about changing them to FOLDER, PARENT, READING_LIST ?

::: mobile/android/base/java/org/mozilla/gecko/home/BookmarkFolderView.java:23
(Diff revision 1)
> +         * A standard folder, i.e. a folder in a list of bookmarks and folders.
> +         */
> +        FOLDER(0),
> +
> +        /**
> +         * A currently opened folder, i.e. this indicates that you are able to return to the previous

Update this comment - I think it makes more sense to refer to this as the parent folder versus "A currently opened folder" because clicking on it takes you back, and the text also says "Go back to ...".

::: mobile/android/base/java/org/mozilla/gecko/home/BookmarkFolderView.java:41
(Diff revision 1)
> +        FolderState(final int state) { this.state = state; }
> +    }
> +
> +    private FolderState mState;
>  
> -    private boolean mIsOpen;
> +    private static final int[] STATE_OPEN = { R.attr.state_open };

I don't think you use this anymore because you reference the attr directly in the enum.

::: mobile/android/base/resources/values/attrs.xml:140
(Diff revision 1)
>               background is full alpha and we need to copy the background underneath. -->
>          <attr name="fadeBackgroundColor" format="dimension"/>
>      </declare-styleable>
>  
>      <declare-styleable name="BookmarkFolderView">
>          <attr name="state_open" format="boolean"/>

Same.
Attachment #8740655 - Flags: review?(liuche) → review+
https://hg.mozilla.org/mozilla-central/rev/98e3ee7643cd
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Verified as fixed in build 48.0a2 2016-04-27;
Device: Asus Transformer Pad (Android 4.2.1).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.