Last Comment Bug 677630 - Extend 'Add to Home Screen' to Bookmark Popup
: Extend 'Add to Home Screen' to Bookmark Popup
Status: VERIFIED FIXED
:
Product: Fennec Graveyard
Classification: Graveyard
Component: Bookmarks (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 8
Assigned To: Mark Finkle (:mfinkle) (use needinfo?)
:
Mentors:
Depends on: 676293
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-09 11:54 PDT by Mark Finkle (:mfinkle) (use needinfo?)
Modified: 2013-07-01 16:37 PDT (History)
3 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (9.08 KB, patch)
2011-08-09 11:54 PDT, Mark Finkle (:mfinkle) (use needinfo?)
mbrubeck: review+
Details | Diff | Review

Description Mark Finkle (:mfinkle) (use needinfo?) 2011-08-09 11:54:01 PDT
Created attachment 551841 [details] [diff] [review]
patch

Bug 676293 added basic support to the Bookmark List context menu. This bug adds it to the Bookmark Popup as well. Based on design here:

http://cl.ly/1P3z27012u3Y00140C1U

The look should mimic the existing context menus.

The patch converts the popup to a contextmenu-ish look, while retaining the arrowbox. It also moves the code to make a bookmark shortcut to BookmarkHelper so it can be easier to reuse.
Comment 1 Matt Brubeck (:mbrubeck) 2011-08-09 16:27:37 PDT
Comment on attachment 551841 [details] [diff] [review]
patch

>+<!ENTITY bookmarkShortcut.label    "Add to Home Screen">

Should this be sentence-case ("Add to home screen")?

>+++ b/mobile/themes/core/gingerbread/browser.css
> #share-title,
>+#bookmark-popup-title,
> #context-hint {
>   color: @color_text_default@;
>   font-size: @font_small@;
>   padding: @padding_small@;
> }

Do we need a similar change in the Froyo theme?  (I saw you and Ian talking on IRC about some additional style tweaks here.)
Comment 2 Mark Finkle (:mfinkle) (use needinfo?) 2011-08-09 18:28:07 PDT
(In reply to Matt Brubeck (:mbrubeck) from comment #1)

> >+<!ENTITY bookmarkShortcut.label    "Add to Home Screen">
>
> Should this be sentence-case ("Add to home screen")?

Nope. Button and Menu labels are title case

> >+++ b/mobile/themes/core/gingerbread/browser.css
> > #share-title,
> >+#bookmark-popup-title,
> > #context-hint {
> >   color: @color_text_default@;
> >   font-size: @font_small@;
> >   padding: @padding_small@;
> > }
> 
> Do we need a similar change in the Froyo theme?  (I saw you and Ian talking
> on IRC about some additional style tweaks here.)

Yep, I added it, tested and showed Ian.
Comment 3 Mark Finkle (:mfinkle) (use needinfo?) 2011-08-09 18:33:47 PDT
http://hg.mozilla.org/mozilla-central/rev/04dfb49d3a3d
Comment 4 Aaron Train [:aaronmt] 2011-08-10 07:56:55 PDT
Verified Fixed
Mozilla/5.0 (Android; Linux armv7l; rv:8.0a1) Gecko/20110810 Firefox/8.0a1 Fennec/8.0a1

Note You need to log in before you can comment on or make changes to this bug.