GNOME Bugzilla – Bug 739836
Improve sidebar and dialog ui
Last modified: 2015-01-10 21:44:28 UTC
Created attachment 290256 [details] [review] Patch Adjust margin, frame,... in sidebar. Simplify properties dialog.
Created attachment 290257 [details] Screenshots
Created attachment 290258 [details] Screenshots
Created attachment 290259 [details] Screenshots
Created attachment 290260 [details] Screenshots
Created attachment 290261 [details] Screenshots
Created attachment 290262 [details] [review] Patch Correct indent
Created attachment 290263 [details] [review] Patch Style FindSidebar
Review of attachment 290263 [details] [review]: Thanks for the patch. I think the patch could be split into several smaller patches, because it actually changes different things. I would also appreciate an explanation in the commit message so that the numbers don't look arbitrary. ::: shell/ev-properties-dialog.c @@ +127,3 @@ + + headerbar = gtk_header_bar_new (); + gtk_header_bar_set_title (GTK_HEADER_BAR (headerbar), "Properties"); Note that "Properties" is a translatable string, it should be marked for translation. But this is not actually needed, we can simply use "use-header-bar" property and the dialog will use a header and use the window title as headerbar title. I've just pushed these changes to git master using the use-header-bar property instead. ::: shell/ev-sidebar-annotations.c @@ +131,3 @@ loading_model = ev_sidebar_annotations_create_simple_model (_("Loading…")); ev_annots->priv->tree_view = gtk_tree_view_new_with_model (loading_model); + gtk_widget_set_margin_top (ev_annots->priv->tree_view, 2); Why 2? and why adding the margin in general? @@ +208,2 @@ item = gtk_toggle_tool_button_new (); + gtk_tool_button_set_icon_name (GTK_TOOL_BUTTON (item), "document-new-symbolic"); So there are three different improvements here, one is adding the margins, another is using relief-none for the tool palette item, and then using symbolic icons. I think we should also use a symbolic icon for the close button in the sidebar. ::: shell/evince.css @@ +39,3 @@ border-style: solid; border-width: 1px; + border-radius: 0; This looks like an unrelated fix, I've just pushed it to git master too. @@ +73,3 @@ + border-width: 0; + border-top-width: 1px; +} Would it be possible to move here all other margin changes? why are some of the changes made in the CSS and some others in the code?
Thank you for the review. This is first time i make patch for GNOME. 1. Thank for commit headerbar support in dialog. 2. "2px" look nice for me. See screenshot for detail. 3. I tried to use margin in css but it doesn't work for me. I don't know why. Therefore, i must do hard code :(
Created attachment 290788 [details] Annotation list without top margin
Created attachment 290789 [details] Annotation list with top margin
Carlos, some changes are in the code since we can't set margins in the css.
Hi. I made new patches for this.
Created attachment 291311 [details] [review] Avoid borders collision in sidebar
Created attachment 291312 [details] [review] Add margin to some areas in sidebar
Created attachment 291313 [details] [review] Set relief-none to annotation tool item
Created attachment 291314 [details] [review] Use linked style for bookmark tool button group
Created attachment 291315 [details] [review] Use symbolic icon in sidebar
Comment on attachment 291311 [details] [review] Avoid borders collision in sidebar Looks good. thanks!
Comment on attachment 291312 [details] [review] Add margin to some areas in sidebar I don't see a big different, but looks good in any case.
Comment on attachment 291313 [details] [review] Set relief-none to annotation tool item Ok.
Comment on attachment 291314 [details] [review] Use linked style for bookmark tool button group Ok
Comment on attachment 291315 [details] [review] Use symbolic icon in sidebar Looks much better, thanks!
Pushed all the patches to git master, thank you!
*** Bug 741819 has been marked as a duplicate of this bug. ***