GNOME Bugzilla – Bug 695313
Design tweaks to match the mockups
Last modified: 2013-03-06 22:23:39 UTC
Going to post a few patches to more closely match the mockups.
Created attachment 238222 [details] [review] city: use icon and text drop shadows
Created attachment 238223 [details] [review] Render an OSD background on the forecast box
Created attachment 238224 [details] [review] Use content view class on forecast sidebar So we can get a noise texture background.
Created attachment 238225 [details] [review] city: add a style class for setting the background under the content view
Created attachment 238226 [details] [review] city: set style class for weather conditions
Created attachment 238227 [details] [review] Move the attribution to the sidebar
Created attachment 238228 [details] [review] forecast: tweak the column and row spacing
Created attachment 238229 [details] [review] city: add a bit more space between the columns
Review of attachment 238222 [details] [review]: Ok
Review of attachment 238223 [details] [review]: ::: src/city.js @@ +64,2 @@ this._forecasts = new Forecast.ForecastBox({ hexpand: true }); + outerGrid.attach(this._forecasts, 0, 1, 2, 1); This change is unrelated, please mention it in the commit message.
Review of attachment 238224 [details] [review]: Ok
Review of attachment 238225 [details] [review]: ::: src/city.js @@ +31,3 @@ this.parent(params); + let outerBox = new Gtk.Box({ orientation: Gtk.Orientation.HORIZONTAL }); Isn't GtkBox deprecated? Should work just as well with a grid. Also, it kind of undoes the previous change to forecastsbox. If that's intentional, they probably need to be squashed.
Review of attachment 238226 [details] [review]: ::: src/city.js @@ +118,3 @@ + if (this._currentStyle) + context.remove_class(this._currentStyle); + this._currentStyle = info.get_icon_name(); info.get_icon_name() probably needs some processing before it generic enough to use as style class. At the very minimum, the moonphase needs to be stripped off.
Review of attachment 238227 [details] [review]: Ok
Review of attachment 238228 [details] [review]: Ok
Review of attachment 238229 [details] [review]: Ok, but I'd phrase it as "add more space to the inner grid", because columns is a bit ambiguous.
Created attachment 238237 [details] [review] city: set style class for weather conditions
Review of attachment 238225 [details] [review]: ::: src/city.js @@ +31,3 @@ this.parent(params); + let outerBox = new Gtk.Box({ orientation: Gtk.Orientation.HORIZONTAL }); Don't know of any effort or desire to deprecate GtkBox. And I don't think this undoes anything related to the forecast box.
Created attachment 238241 [details] [review] Render an OSD background on the forecast box And change the grid packing to ensure box spans the view.
Created attachment 238242 [details] [review] city: set style class for weather conditions
Created attachment 238246 [details] [review] city: use OSD style for revealer button
Created attachment 238247 [details] [review] city: add right margin on revealer button
Created attachment 238248 [details] [review] city: add padding all the way around conditions image This helps work around a bug in the way icon shadows are done.
Created attachment 238249 [details] [review] city: override the backdrop color for the conditions content
(In reply to comment #18) > Review of attachment 238225 [details] [review]: > > ::: src/city.js > @@ +31,3 @@ > this.parent(params); > > + let outerBox = new Gtk.Box({ orientation: Gtk.Orientation.HORIZONTAL > }); > > Don't know of any effort or desire to deprecate GtkBox. And I don't think this > undoes anything related to the forecast box. Well, if you move the sidebar from the outer grid to a separate box, the outer grid has only one column left, so the change to call attach() with a width of 2 on the forecast box (patch 238241) becomes wrong.
Review of attachment 238241 [details] [review]: ::: src/city.js @@ +64,2 @@ this._forecasts = new Forecast.ForecastBox({ hexpand: true }); + outerGrid.attach(this._forecasts, 0, 1, 2, 1); I though the purpose of this change was to have the forecast box below the sidebar, but now I don't understand it.
Review of attachment 238242 [details] [review]: Looks good.
Review of attachment 238241 [details] [review]: On a second though, the outer grid has three columns now (inner grid, button, sidebar), so this change is correct (if you ignore the vertical alignment of the button)
Review of attachment 238246 [details] [review]: Yes.
Review of attachment 238247 [details] [review]: OK
Review of attachment 238248 [details] [review]: Doesn't this break the vertical alignment in the inner grid?
Review of attachment 238249 [details] [review]: Ok
Created attachment 238257 [details] [review] city: add padding all the way around conditions image Added padding to the labels to keep the vertical aligment
Review of attachment 238257 [details] [review]: Perfect!
Attachment 238222 [details] pushed as cbafc1f - city: use icon and text drop shadows Attachment 238224 [details] pushed as caca3d2 - Use content view class on forecast sidebar Attachment 238225 [details] pushed as becfb32 - city: add a style class for setting the background under the content view Attachment 238227 [details] pushed as 5bc3b65 - Move the attribution to the sidebar Attachment 238228 [details] pushed as 479e724 - forecast: tweak the column and row spacing Attachment 238241 [details] pushed as 958f383 - Render an OSD background on the forecast box Attachment 238242 [details] pushed as 9a4f60b - city: set style class for weather conditions Attachment 238246 [details] pushed as 5675fce - city: use OSD style for revealer button Attachment 238247 [details] pushed as 6e23539 - city: add right margin on revealer button Attachment 238249 [details] pushed as cf5adbf - city: override the backdrop color for the conditions content Attachment 238257 [details] pushed as 55baf29 - city: add padding all the way around conditions image