GNOME Bugzilla – Bug 655627
Menu popups should match the mockups
Last modified: 2011-08-02 10:05:57 UTC
Created attachment 192906 [details] [review] Theme tweaks patch There are a few minor discrepancies at the moment which means that the menu popups don't look as good as they could. The attached patch includes the necessary theme changes. The other thing that needs doing is a 2px vertical gap between the panel edge and the tip of the boxpointer
Review of attachment 192906 [details] [review]: Sure. The commit message could be improved; at the very least, the bug URL needs to be added (on its own line at the end, separated by a blank line from the preceding block). Though ideally, it takes the form of subject: summary of the change A body providing more details about the change and the reasoning for it. http://bugzilla.gnome.org/show_bug.cgi?id=foobar Tip of the day: use git-bz (from Owen's blog), it makes attaching patches to bugzilla suck quite a bit less (and also takes care of appending the bug URL).
Created attachment 192910 [details] [review] Revised theme patch Thanks for the advice Florian. I hope this is better.
Review of attachment 192910 [details] [review]: The body of the commit message needs to be split into multiple lines (~75 characters each), otherwise good to push. (You didn't really explain the reasoning behind any of the changes - it's not common for style changes, but personally I'd find it quite educational)
Other things about the commit message: the literal word "Subject" wasn't intended. And like the English rule about how you never say "this essay explains why" in essays, "this patch" is similarly bad. theme: Improve menu boxpointer arrow styling Make the menu popups fully match the mockups by changing the borders and dimensions.
(In reply to comment #3) > Review of attachment 192910 [details] [review]: > > The body of the commit message needs to be split into multiple lines (~75 > characters each), otherwise good to push. > (You didn't really explain the reasoning behind any of the changes - it's not > common for style changes, but personally I'd find it quite educational) Done done and done: http://git.gnome.org/browse/gnome-shell/commit/?id=acedb60abe032dc0521c27a395d0d28f05c80642 Do we need a separate bug for adding the gap between the pointer and panel edge?
Created attachment 192915 [details] [review] panelMenu: add a gap between the panel and its menus The specs call for a 2 pixel gap between the panel and its menus, though we need to specify this as 4 pixels, since it's relative to the bottom of the icon/title, not the bottom of the panel (up until now, the point of the menu arrow was actually overlapping the menu's highlight underline). ==== does this look right?
Review of attachment 192915 [details] [review]: Could we expose the gap in the style? It's a tad bit ugly to assume certain style properties ("2 pixel padding between icon and panel bottom") in the code ...
(In reply to comment #6) ... > > does this look right? I've tested the patch. It looks good.
Created attachment 192987 [details] [review] panelMenu: add a gap between the panel and its menus now with CSS!
Review of attachment 192987 [details] [review]: Good to commit after updating the commit message to mention that the gap parameter was moved to CSS.
Attachment 192987 [details] pushed as 2403fd0 - panelMenu: add a gap between the panel and its menus
Created attachment 193015 [details] [review] Message tray and panel box pointers should match The message tray and panel box pointers should be themed in the same way.
Review of attachment 193015 [details] [review]: Sure.
(In reply to comment #13) > Review of attachment 193015 [details] [review]: > > Sure. Pushed: http://git.gnome.org/browse/gnome-shell/commit/?id=aba5f2f7b8fa175ce81e464ef999722dd35cae20