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 745530 - Theme: refactoring stylesheet
Theme: refactoring stylesheet
Status: RESOLVED FIXED
Product: gnome-maps
Classification: Applications
Component: general
git master
Other All
: Normal minor
: ---
Assigned To: gnome-maps-maint
gnome-maps-maint
Depends on:
Blocks:
 
 
Reported: 2015-03-03 12:55 UTC by Miguel Vaello Martínez
Modified: 2015-03-05 19:37 UTC
See Also:
GNOME target: ---
GNOME version: 3.15/3.16


Attachments
Refactoring stylesheet (1.81 KB, patch)
2015-03-03 13:01 UTC, Miguel Vaello Martínez
none Details | Review
The good one patch (1.81 KB, patch)
2015-03-03 20:42 UTC, Miguel Vaello Martínez
committed Details | Review

Description Miguel Vaello Martínez 2015-03-03 12:55:16 UTC
This trivial patch makes a code easier to read and maintain (refactor), and generates a smaller final file.
Comment 1 Miguel Vaello Martínez 2015-03-03 13:01:50 UTC
Created attachment 298423 [details] [review]
Refactoring stylesheet
Comment 2 Jonas Danielsson 2015-03-03 13:05:50 UTC
Review of attachment 298423 [details] [review]:

Thanks for the patch!

This looks really nice.  I do not know a lot of CSS tho, have you verified that this does not change anything?

::: data/gnome-maps.css
@@ +9,3 @@
 .layer-radio-button {
     background-image: none;
+    padding: 0;

Why remove the px here? You keep the px for the other padding attributes below.
And other attributes also has it.
Comment 3 Miguel Vaello Martínez 2015-03-03 14:08:01 UTC
Oh, yes, this is because zero (0) doesn't matter if is 0px or 0em, zero is "adimensional" in CSS. But is mandatory put px (or whatever) in (X)px where X >= 1.
Comment 4 Jonas Danielsson 2015-03-03 18:51:40 UTC
Review of attachment 298423 [details] [review]:

Did this run ok for you?

I got:
(org.gnome.Maps:31466): Gjs-WARNING **: JS ERROR: Gtk.CssProviderError: application.css:62:13 Expected a length

::: data/gnome-maps.css
@@ +61,2 @@
 #instruction-box > GtkImage {
+    padding: auto 6px;

Is auto really ok to use here?
Comment 5 Miguel Vaello Martínez 2015-03-03 19:55:20 UTC
(In reply to Jonas Danielsson from comment #4)
> Review of attachment 298423 [details] [review] [review]:
> 
> Did this run ok for you?
> 
> I got:
> (org.gnome.Maps:31466): Gjs-WARNING **: JS ERROR: Gtk.CssProviderError:
> application.css:62:13 Expected a length
> 
> ::: data/gnome-maps.css
> @@ +61,2 @@
>  #instruction-box > GtkImage {
> +    padding: auto 6px;
> 
> Is auto really ok to use here?

Ok, I think is my fault. I was confused about this error after see "application.css", then I thought to myself: "looks like not the same stylesheet". I didn't find the mentioned stylesheet and I thought it could be due by an other bug.

I think that Gtk CSS support is not quite "complete" as I thought and I made the patch without testing properly. But on the other hand, I don't understand why Gtk CSS do not accept auto, I have reviewed the docs and I didn't find any alternative for auto. Auto is something like "default" or inherited value.

I'm going to workaround the issue and upload again the patch. I am sorry for the inconvenience.
Comment 6 Miguel Vaello Martínez 2015-03-03 20:42:54 UTC
Created attachment 298481 [details] [review]
The good one patch