After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 695313 - Design tweaks to match the mockups
Design tweaks to match the mockups
Status: RESOLVED FIXED
Product: gnome-weather
Classification: Applications
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: GNOME Weather Maintainer(s)
GNOME Weather Maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2013-03-06 18:36 UTC by William Jon McCann
Modified: 2013-03-06 22:23 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
city: use icon and text drop shadows (665 bytes, patch)
2013-03-06 18:37 UTC, William Jon McCann
committed Details | Review
Render an OSD background on the forecast box (3.61 KB, patch)
2013-03-06 18:37 UTC, William Jon McCann
accepted-commit_now Details | Review
Use content view class on forecast sidebar (965 bytes, patch)
2013-03-06 18:37 UTC, William Jon McCann
committed Details | Review
city: add a style class for setting the background under the content view (2.08 KB, patch)
2013-03-06 18:37 UTC, William Jon McCann
committed Details | Review
city: set style class for weather conditions (1.28 KB, patch)
2013-03-06 18:37 UTC, William Jon McCann
reviewed Details | Review
Move the attribution to the sidebar (4.13 KB, patch)
2013-03-06 18:37 UTC, William Jon McCann
committed Details | Review
forecast: tweak the column and row spacing (1.08 KB, patch)
2013-03-06 18:37 UTC, William Jon McCann
committed Details | Review
city: add a bit more space between the columns (1.07 KB, patch)
2013-03-06 18:37 UTC, William Jon McCann
accepted-commit_now Details | Review
city: set style class for weather conditions (1.71 KB, patch)
2013-03-06 19:46 UTC, William Jon McCann
none Details | Review
Render an OSD background on the forecast box (3.75 KB, patch)
2013-03-06 20:30 UTC, William Jon McCann
committed Details | Review
city: set style class for weather conditions (2.14 KB, patch)
2013-03-06 20:31 UTC, William Jon McCann
committed Details | Review
city: use OSD style for revealer button (1004 bytes, patch)
2013-03-06 21:06 UTC, William Jon McCann
committed Details | Review
city: add right margin on revealer button (973 bytes, patch)
2013-03-06 21:06 UTC, William Jon McCann
committed Details | Review
city: add padding all the way around conditions image (700 bytes, patch)
2013-03-06 21:06 UTC, William Jon McCann
reviewed Details | Review
city: override the backdrop color for the conditions content (1.34 KB, patch)
2013-03-06 21:06 UTC, William Jon McCann
committed Details | Review
city: add padding all the way around conditions image (866 bytes, patch)
2013-03-06 22:11 UTC, William Jon McCann
committed Details | Review

Description William Jon McCann 2013-03-06 18:36:35 UTC
Going to post a few patches to more closely match the mockups.
Comment 1 William Jon McCann 2013-03-06 18:37:31 UTC
Created attachment 238222 [details] [review]
city: use icon and text drop shadows
Comment 2 William Jon McCann 2013-03-06 18:37:34 UTC
Created attachment 238223 [details] [review]
Render an OSD background on the forecast box
Comment 3 William Jon McCann 2013-03-06 18:37:36 UTC
Created attachment 238224 [details] [review]
Use content view class on forecast sidebar

So we can get a noise texture background.
Comment 4 William Jon McCann 2013-03-06 18:37:39 UTC
Created attachment 238225 [details] [review]
city: add a style class for setting the background under the content view
Comment 5 William Jon McCann 2013-03-06 18:37:41 UTC
Created attachment 238226 [details] [review]
city: set style class for weather conditions
Comment 6 William Jon McCann 2013-03-06 18:37:43 UTC
Created attachment 238227 [details] [review]
Move the attribution to the sidebar
Comment 7 William Jon McCann 2013-03-06 18:37:46 UTC
Created attachment 238228 [details] [review]
forecast: tweak the column and row spacing
Comment 8 William Jon McCann 2013-03-06 18:37:49 UTC
Created attachment 238229 [details] [review]
city: add a bit more space between the columns
Comment 9 Giovanni Campagna 2013-03-06 18:50:48 UTC
Review of attachment 238222 [details] [review]:

Ok
Comment 10 Giovanni Campagna 2013-03-06 18:52:33 UTC
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.
Comment 11 Giovanni Campagna 2013-03-06 18:52:56 UTC
Review of attachment 238224 [details] [review]:

Ok
Comment 12 Giovanni Campagna 2013-03-06 18:56:04 UTC
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.
Comment 13 Giovanni Campagna 2013-03-06 18:59:54 UTC
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.
Comment 14 Giovanni Campagna 2013-03-06 19:01:47 UTC
Review of attachment 238227 [details] [review]:

Ok
Comment 15 Giovanni Campagna 2013-03-06 19:02:17 UTC
Review of attachment 238228 [details] [review]:

Ok
Comment 16 Giovanni Campagna 2013-03-06 19:03:53 UTC
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.
Comment 17 William Jon McCann 2013-03-06 19:46:03 UTC
Created attachment 238237 [details] [review]
city: set style class for weather conditions
Comment 18 William Jon McCann 2013-03-06 19:49:29 UTC
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.
Comment 19 William Jon McCann 2013-03-06 20:30:27 UTC
Created attachment 238241 [details] [review]
Render an OSD background on the forecast box

And change the grid packing to ensure box spans the view.
Comment 20 William Jon McCann 2013-03-06 20:31:49 UTC
Created attachment 238242 [details] [review]
city: set style class for weather conditions
Comment 21 William Jon McCann 2013-03-06 21:06:32 UTC
Created attachment 238246 [details] [review]
city: use OSD style for revealer button
Comment 22 William Jon McCann 2013-03-06 21:06:35 UTC
Created attachment 238247 [details] [review]
city: add right margin on revealer button
Comment 23 William Jon McCann 2013-03-06 21:06:37 UTC
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.
Comment 24 William Jon McCann 2013-03-06 21:06:40 UTC
Created attachment 238249 [details] [review]
city: override the backdrop color for the conditions content
Comment 25 Giovanni Campagna 2013-03-06 21:46:50 UTC
(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.
Comment 26 Giovanni Campagna 2013-03-06 21:48:22 UTC
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.
Comment 27 Giovanni Campagna 2013-03-06 21:49:04 UTC
Review of attachment 238242 [details] [review]:

Looks good.
Comment 28 Giovanni Campagna 2013-03-06 21:51:02 UTC
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)
Comment 29 Giovanni Campagna 2013-03-06 21:51:17 UTC
Review of attachment 238246 [details] [review]:

Yes.
Comment 30 Giovanni Campagna 2013-03-06 21:51:32 UTC
Review of attachment 238247 [details] [review]:

OK
Comment 31 Giovanni Campagna 2013-03-06 21:52:26 UTC
Review of attachment 238248 [details] [review]:

Doesn't this break the vertical alignment in the inner grid?
Comment 32 Giovanni Campagna 2013-03-06 21:53:06 UTC
Review of attachment 238249 [details] [review]:

Ok
Comment 33 William Jon McCann 2013-03-06 22:11:54 UTC
Created attachment 238257 [details] [review]
city: add padding all the way around conditions image

Added padding to the labels to keep the vertical aligment
Comment 34 Giovanni Campagna 2013-03-06 22:14:45 UTC
Review of attachment 238257 [details] [review]:

Perfect!
Comment 35 William Jon McCann 2013-03-06 22:23:12 UTC
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