Closed Bug 695199 Opened 8 years ago Closed 8 years ago

Bookmarking: Add/Remove from Menu

Categories

(Firefox for Android :: General, defect, P1)

ARM
Android
defect

Tracking

()

VERIFIED FIXED
Tracking Status
firefox11 --- fixed
fennec 11+ ---

People

(Reporter: elan, Assigned: sriram)

Details

(Whiteboard: [birch] [ux needed] [Product Approve])

Attachments

(2 files, 1 obsolete file)

* need UI design
Assignee: nobody → lucasr.at.mozilla
Assignee: lucasr.at.mozilla → kgupta
Summary: Bookmarking → Bookmarking: Add/List/Remove
Whiteboard: [birch] [ux needed]
This including Add/Remove and *Edit*?
Aaron: Product and Eng have decided no Edit Functionality at this point.
Whiteboard: [birch] [ux needed] → [birch] [ux needed] [Product Approve]
Attached patch WIP (obsolete) — Splinter Review
This patch does add/remove bookmarks through the options menu.
When gecko is loading, since the URL is not known, both the options are hidden.
Once gecko loads and URL is known to Tabs, add or remove option is shown based on the presence/absence of bookmark.

Todo:
Edit option is currently not implemented. This needs UI spec.
Assignee: kgupta → sriram
Attachment #569563 - Flags: review?(mark.finkle)
Comment on attachment 569563 [details] [diff] [review]
WIP

* Do we need to use two menu items? Can we dynamically change the image state?
* Original designs only want to use "Bookmark" as the text, and the image would change depending on the state. I'm not sure if we can do that on ICS (menus with images). This patch uses "Add Bookmark" / "Remove Bookmark"

Can you rebase the patch so I can try it out?
Note: ICS will use images in the action bar (ifRoom), just not sure about the dropdown menu
Attached patch PatchSplinter Review
This patch adds just one entry in menu and changes it based on whether a bookmark is already added or not.
A toast message is shown based on add/remove action.

Todo:
Finalizing the string for the toast.
Replacing the icon assets.
Attachment #569563 - Attachment is obsolete: true
Attachment #569563 - Flags: review?(mark.finkle)
Attachment #569770 - Flags: review?(mark.finkle)
Attachment #569770 - Flags: review?(mark.finkle) → review+
Attached patch PatchSplinter Review
Refreshed patch.
pushed rebased patch
https://hg.mozilla.org/projects/birch/rev/97403e114b58
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Summary: Bookmarking: Add/List/Remove → Bookmarking: Add/Remove from Menu
I think for future l10n translations, the strings added in the patch above should remove the exclamation point.

"Bookmark added successfully!" -> "Saved to bookmarks."
"Bookmark removed!" -> "Removed bookmark." or "Bookmark removed."
(In reply to Aaron Train [:aaronmt] from comment #9)
> I think for future l10n translations, the strings added in the patch above
> should remove the exclamation point.
> 
> "Bookmark added successfully!" -> "Saved to bookmarks."
> "Bookmark removed!" -> "Removed bookmark." or "Bookmark removed."

Damn! I agree completely and I forgot to change those before landing. Just did a quick change:

https://hg.mozilla.org/projects/birch/rev/adad73e2ee6f
Mozilla/5.0 (Android; Linux armv7l; rv:10.0a1) Gecko/20111027 Firefox/10.0a1 Fennec/10.0a1
Status: RESOLVED → VERIFIED
tracking-fennec: --- → 11+
You need to log in before you can comment on or make changes to this bug.