436260 - Implementation of timezones display should be changed

Status: RESOLVED FIXED

(Calendar :: Calendar Frontend, defect)

Description

[Berend Cornelius]

Currently our implementation to display the timezones is suboptimal in various aspects:
1) We keep all the images duplicately once for pinstripe and once for winstripe. If we should decide one day to display different images for the platforms we can still separate them easily.
2) All the images are part of a stack container and during runtime it is decided which image to be displayed - all others are hidden. This is also suboptimal. It would be a better solution to put all images into one and control the timezone display via css- style attribute "moz-image-region".
For further info please see
./prototypes/themes/pinstripe|winstripe
./prototypes/wcap/sun-calendar-dialog.timezone.js
./prototypes/wcap/sun-calendar-dialog.timezone.xul

Comment 1

[Philipp Kewisch [:Fallen][☁️📆]]

Attachment: Timezones image

This is the timezones image, created using imagemagick's convert and the +append command. Its probably best to use a script to create the moz image rules.

If I did everything right a simple script like:

for i in `seq 0 38`; do i1=$(($i+1)); echo "-moz-image-region: rect(0 $(($i1*460))px 287px $(($i*460))px);"; done

should create the rule bodies for the image. You just need to add the css selectors, switch the stack to a single image and make sure it works.

Comment 2

[ovidiu]

related ? bug 302253. or obsolete? (I think that one can become rfe ..)

Comment 3

[anirvana]

Attachment: Timezone image

I have attached the timezone image i have used,the earlier image lacked some portion corresponding to timezone -0800.

Comment 4

[anirvana]

Attachment: Attached a patch. Please review it.

Now both the compact timezones image and world map image are placed inside the common/ folder.

Comment 5

[Philipp Kewisch [:Fallen][☁️📆]]

Comment on attachment 430662 [details] [diff] [review]
Attached a patch. Please review it.

First a few general style comments, see also https://wiki.mozilla.org/Calendar:Style_Guide

* If changing a function that uses var, please change the "var" to "let".
* Make sure indentation is correct. This means use spaces instead of tabs and that lines of the same level are aligned.
* When passing parameters, add a space after a comma (i.e foo("bar", "baz");

* You move/delete a lot of files. This is fine, but you need to tell hg this is happening. The following commands allow hg to remember that the files are moved or deleted and they will show up in the patch.

  hg mv  /path/to/source /path/to/destination
  hg rm  /path/to/file

* Similarly, when you add a file, instead of attaching it to this bug separately, you can use the following command to have hg record it in the patch, which makes review and checkin easier.

  hg add /path/to/file

>     var stack = document.getElementById("timezone-stack");
>+    var numChilds = stack.childNodes.length; 
>+        var image = stack.childNodes[1];
>+        image.setAttribute("tzid",standardTZOffset);
>+        image.setAttribute("offset","19800");
>+        image.removeAttribute("hidden");
It seems you don't use numChilds here, please remove that variable. Also, instead of getting the image via childNodes, please give the image an id and then use getElementById.





>+<?xml-stylesheet type="text/css" href="chrome://calendar/content/calendar-timezone-highlighter.css"?> 
CSS files should live in /skin/



>   <stack id="timezone-stack">
>+    <image class="timezone-highlight" offset="0" tzid="+0000" hidden="true"/>
>+     </stack>
Here the image node to give an id. Since we'll always be showing a highlight, we probably don't have to set hidden=true?


> % content calendar %content/calendar/
> % style chrome://calendar/content/calendar-event-dialog.xul chrome://global/skin/toolbar.css
>     content/calendar/agenda-listbox.js                     (content/agenda-listbox.js)
>+                                                           
>     content/calendar/agenda-listbox.xml                    (content/agenda-listbox.xml)
>     content/calendar/calErrorPrompt.xul                    (content/calErrorPrompt.xul)
No need to add a blank line here.

>+    content/calendar/calendar-timezone-highlighter.css     (content/dialogs/calendar-timezone-highlighter.css)
As mentioned, this file should rather live in base/themes/common/dialogs/ and referenced in jar.mn appropriatly.


>-    skin/calendar/widgets/minimonth.css                    (themes/@THEME@/widgets/minimonth.css)
>+    skin/calendar/widgets/minimonth.css                    (themes/@THEME@/widgets/minimonth.css) 
Please update your repository, this file was moved into base/themes/common I believe.



>diff -r b3c58a540469 calendar/base/themes/pinstripe/dialogs/calendar-event-dialog.css

>+ 
>\ No newline at end of file
No need to add a blank line here.


>diff -r b3c58a540469 calendar/base/themes/winstripe/dialogs/calendar-event-dialog.css
>  *
>  * ***** END LICENSE BLOCK ***** */
>+ 
> 
No need to add a blank line here.



I'm going to r- this patch for now to fix the mentioned issues, please take care of the comments and upload a new patch.

Comment 6

[anirvana]

Attachment: I have tried to remove the previous errors this time.Please review it.

Comment 7

[Philipp Kewisch [:Fallen][☁️📆]]

Comment on attachment 433714 [details] [diff] [review]
I have tried to remove the previous errors this time.Please review it.

>+    image.setAttribute("tzid",standardTZOffset);     
Missing space after comma.
> 
>   <stack id="timezone-stack">
>+     <image src="chrome://calendar/skin/timezone_map.png"/>
>+    <image class="timezone-highlight" tzid="+0000" id="highlighter"/>
>+     </stack>
Indentation is still wrong here.

>     skin/calendar/toolbar-small.png                        (themes/@THEME@/images/toolbar-small.png)
>-    skin/calendar/widgets/calendar-widgets.css             (themes/@THEME@/widgets/calendar-widgets.css)
>-    skin/calendar/timezone_map.png                         (themes/@THEME@/dialogs/images/timezone_map.png)
>+    skin/calendar/widgets/calendar-widgets.css             (themes/@THEME@/widgets/calendar-widgets.css)      
Seems you are removing timezone_map from the jar.mn file?


>+.timezone-highlight[tzid="none"] {
>+  display:none;
Space after :

>+  -moz-image-region: rect(0 460px 287px 0px);
>+}
>+
>+
>+
Remove Extra newlines

>diff -r d46ad9748f3f calendar/base/themes/common/images/timezone_map.png
>Binary file calendar/base/themes/common/images/timezone_map.png has changed
It seems you are not including the binary data. Please make sure you have git style patches enabled in your hgrc, so the binary data of the images is included and removals are recorded.

Comment 8

[anirvana]

Attachment: Another patch.

The timezones_map image hasn't been removed from jar.mn rather it has been placed within the commom/ folder.That's why it has been mentioned up in the jar.mn file

Comment 9

[Philipp Kewisch [:Fallen][☁️📆]]

Comment on attachment 433898 [details] [diff] [review]
Another patch.

Almost there now! The patch applies and works as expected, so r+ from a functionality point of view. A few more minor bits that need fixing so I can check it in. Most of these are mentioned in the style guide:

b/calendar/base/content/dialogs/calendar-event-dialog-timezone.js:147: Trailing whitespace
+    image.setAttribute("tzid", standardTZOffset);     

b/calendar/base/jar.mn:113: Trailing whitespace
+    skin/calendar/calendar-timezone-highlighter.css        (themes/common/dialogs/calendar-timezone-highlighter.css)  

b/calendar/base/jar.mn:131: Trailing whitespace
+    skin/calendar/timezones.png                            (themes/common/images/timezones.png) 

b/calendar/base/jar.mn:155: Trailing whitespace
+    skin/calendar/widgets/calendar-widgets.css             (themes/@THEME@/widgets/calendar-widgets.css)      

b/calendar/base/themes/common/dialogs/calendar-timezone-highlighter.css:1: Trailing whitespace
+.timezone-highlight { 

b/calendar/base/themes/common/dialogs/calendar-timezone-highlighter.css:24: Trailing whitespace
+} 

b/calendar/base/themes/common/dialogs/calendar-timezone-highlighter.css:67: Trailing whitespace
+.timezone-highlight[tzid="+1130"] { 

b/calendar/base/themes/common/dialogs/calendar-timezone-highlighter.css: Missing License Header

You can get the missing license header at http://www.mozilla.org/MPL/boilerplate-1.1/

Comment 10

[anirvana]

Attachment: Another patch.

Should be the last patch.

Comment 11

[Philipp Kewisch [:Fallen][☁️📆]]

Comment on attachment 433898 [details] [diff] [review]
Another patch.

When uploading new patches, please mark the old patch as obsolete and give the new patch a different description (i.e Fix - v1, then Fix - v2, ...)

This makes it easier to see what is still relevant.

Comment 12

[Philipp Kewisch [:Fallen][☁️📆]]

Comment on attachment 434201 [details] [diff] [review]
Another patch.

Looks good, r=philipp

Comment 13

[Philipp Kewisch [:Fallen][☁️📆]]

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/030391fd4867>

-> FIXED