Closed Bug 444292 Opened 16 years ago Closed 16 years ago

Replace Decorated Header (Nav Bar) by standard Items

Categories

(Calendar :: Calendar Frontend, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: berend.cornelius09, Assigned: berend.cornelius09)

References

Details

(Keywords: late-l10n)

Attachments

(9 files, 1 obsolete file)

As being discussed on the face2face meeting in Hamburg (see http://wiki.mozilla.org/Calendar:Hamburg_2008_-_F2F_Meeting) 
we want to redesign our views (see 
http://wiki.mozilla.org/Calendar:Improving_the_Calendar_Views
and
http://wiki.mozilla.org/Calendar:Lightning09#Calendar_Views

The new decorated header is to contain navigation buttons, date Info and Calendar Week Info and should appear more "tidy up than the current decorated header.
Flags: wanted-calendar0.9?
Attached image new navigation buttons
This png contains the images for the navigation buttons and conforms to the navigation buttons already used in the miniday and minimonth panes. It must be copied into the calendar/base/themes/common/widgets folder
Attached patch patch v. #1 (obsolete) — Splinter Review
first version of this issue. I already gave it to Andreas to test it and he found it Ok. The header is implemented for the rotated view, too. What is still improveable is the display of the date ranges on the left hand side of the decorated header. I implemented it with our "ordinary" "formatInterval" function of the "datetime-formatter" but Christian (and me too) was not satisfied with the result. I'd like to find a better solution in a second patch.
Attachment #328640 - Flags: ui-review?(christian.jansen)
Attachment #328640 - Flags: review?(philipp)
Attached image screenshot
screenshot of the new decorated header with my first patch being applied.
Christian gave me some instructions on how to format the date-range display:
...
~~~ Day ~~~

    EN: March 3, 2008
    DE: 3. März 2008


~~~ Week ~~~

    EN: March 3 - 9, 2008
    DE: 3 - 9. März 2008

    EN: September 29 - October 5, 2008
    DE: 29. September - 5.- October 2008

    EN: December 29, 2008 - January 4, 2009
    DE: 29. Dezember 2008 - 4. Januar 2009


~~~ Multiweek (in this example 2 weeks) ~~~

    EN: March 3 - 16, 2008
    DE: 3 - 16. März 2008

    EN: September 29 - October 12, 2008
    DE: 29. September - 12.- Oktober 2008

    EN: December 29, 2008 - January 11, 2009
    DE: 29. Dezember 2008 - 11. Januar 2009


~~~ Month ~~~

    EN: March 2008
    DE: März 2008


For the hyphen we should use "–" or in Unicode "–"
...

I am not sure what kind of localization problems come up when we implement these formats and would therefor welcome input from the community. I am going to get some advice from an OpenOffice-Calc developer in the coming days. 
Probably I am also going to modify "formatInterval" in the file calendar/base/src/calDateTimeFormatter.js which would mean that I have to adapt the client code at several other places.
Flags: wanted-calendar0.9? → wanted-calendar0.9+
(In reply to comment #4)
If you are modifying "formatInterval" please consider Bug 413103 too.

Regarding localization problems: The interval string should probably defined in a l10n file, e.g. like "%1 - %2". This way the l10n teams could swap the order of start and end date or replace the "-" with a more appropriate character.
I think the first calendar weeks in the year should be displayed in two digits format. Because my first impression when I traveled in the multiweekview across the year change and I saw 'Calendar weeks 51-2' was "Oh, the leading 5 is gone -> bug". To prevent this it would be better to display a '02'.
Not that I do ui-review, but I dislike buttons that are gray in their normal state. While its fine to have hover/active be a different color, I think its visually distracting to have it always be gray.
Status: NEW → ASSIGNED
Comment on attachment 328640 [details] [diff] [review]
patch v. #1

>     skin/classic/calendar/widgets/nav-buttons.png (themes/common/widgets/nav-buttons.png)
>+    skin/classic/calendar/widgets/view-navigation.png (themes/common/widgets/view-navigation.png)
These images are obviously quite similar. I'd prefer to have them in one file when checking in, especially since the filenames are quite similar too.

>+                             tooltiptext="previous day"
>+                             tooltiptext="today"
>+                             tooltiptext="next day"
Please get rid of hardcoded strings. I've noticed some tooltip text gets set later on, so we probably don't need those strings anyway?

>+                            secondWeekNo = getWeekFormatter().getWeekTitle(aEndDate);                            
This line has redundant spaces, there was at least one other line somewhere else.

>+                    document.getAnonymousElementByAttribute(this, "anonid", "nav-control").setDateRange(aStartDate, aEndDate, aToolTipTexts, viewElement);
Wrapping would be nice here.

>+                        d1.day += 7*aNumber;
Spaces around operator

>+          this.fireEvent("relayouted");
I think just "relayout" sounds more natural, but thats up to you.

>+function calGetStringArray(aBundleName, aStringNames, aParams, aComponent) {
Documentation please.

> calendar-navigation-buttons { 
>+  padding-top: 7px;
>+  padding-bottom: 4px;
>+  background-color: white;
white? isn't there some -moz-... color we can use here?

>+.view-header {
>+    font-weight: bold;
>+    font-size: 15px;
>+    color: #616163;
Same question here? looks almost like gray, so maybe also a -moz color?

>+.view-navigation-button {
>+  list-style-image: url("chrome://calendar/skin/widgets/view-navigation.png");
>+  -moz-user-focus: normal;
>+}
Quotes around the url are not needed

>Index: locales/en-US/chrome/calendar/global.dtd
...
>+<!ENTITY onemonthbackward.tooltip "One Month Back" >
>+<!ENTITY onemonthforward.tooltip "One Month Forward" >
>+<!ENTITY showToday.tooltip "Go to Today">
>+<!ENTITY onedayforward.tooltip "One Day Forward">
>+<!ENTITY onedaybackward.tooltip "One Day Backward">
>+<!ENTITY showselectedday.tooltip "Show events for selected day">

Do we really use these globally? Or are they maybe better kept in some view specific .dtd ?

r=philipp
Attachment #328640 - Flags: review?(philipp) → review+
> >+                             tooltiptext="previous day"
> >+                             tooltiptext="today"
> >+                             tooltiptext="next day"
> Please get rid of hardcoded strings. I've noticed some tooltip text gets set
> later on, so we probably don't need those strings anyway?
> 

I suggest to use more instructing tooltips like:

Mini Day

    One Day Back
    Go to Today
    One Day Forward

Mini Month

    One Month Back
    Go to Today
    One Month Forward


Day View

    One Day Back
    Go to Today
    One Day Forward

Week View

    One Week Back
    Go to Today
    One Week Forward

Multiweek

    One Week Back
    Go to Today
    One Week Forward

Month

    One Month Back
    Go to Today
    One Month Forward
(In reply to comment #7)
> Not that I do ui-review, but I dislike buttons that are gray in their normal
> state. While its fine to have hover/active be a different color, I think its
> visually distracting to have it always be gray.
> 

Good point. I think I'll replace them by black nav buttons.
in reply to commment #9: The hardcoded strings are merely relicts from the first implementation phase and I just forgot to remove them. My patch already contains the strings from Christian.
(In reply to comment #10)
> (In reply to comment #7)
> > Not that I do ui-review, but I dislike buttons that are gray in their normal
> > state. While its fine to have hover/active be a different color, I think its
> > visually distracting to have it always be gray.
> > 
> 
> Good point. I think I'll replace them by black nav buttons.

See attachment
> 

Attached image revised button color
Comment on attachment 328641 [details]
screenshot

Looks good for me.
Please try to fix the problem that the buttons jump if one switches from "Week" to "Day", or "Today". Please also change the color used for the date info to #2E4E73.
The date formating should be fixed in a spin-off bug.

ui=christian
Attachment #328641 - Flags: ui-review+
I worked in the comments as suggested by Philipp and Christian. Some notes:

>These images are obviously quite similar. I'd prefer to have them in one file
>when checking in, especially since the filenames are quite similar too.
I don't think this is a good idea since the images have different sizes, so that the moz-image-region property of all images has to be adapted each time one of the images has been modified.

>white? isn't there some -moz-... color we can use here?
As far as I understood
http://developer.mozilla.org/en/docs/CSS:color_value
these common color keyworks are the ones to be used.

> Same question here? looks almost like gray, so maybe also a -moz color?
No Christian insists to use his own color definitions. In this case the font-color has to be adapted to the image color.


>Do we really use these globally? Or are they maybe better kept in some view
>specific .dtd ?
I was unsure myself. I used the global.dtd because it was the only one that was already used by the minimonth. On the other hand I find these tooltips "globally" usable for the future.

I tried to address the "jumping" of comment #18 by setting the padding *before* the label was set and found this improved the situation partly. What else can I do?

One last note about trailing whitespaces. I have recently tended to remove them shortly before the check-in and then remove all whitespaces from the files with the respective function of my editor - at least with files that are not modified frequently. Removing them before the review makes the patch unnecessary big, unclear and likely to get bitrotten.  I hope this is Ok and of course I still appreciatereviewers pointing to that problem.

I removed the "prevnextarrow.png" files in the base/themes/pinstripe|winstripe folders as they are no longer needed

modified patch checked in on trunk and MOZILLA_1_8_BRANCH
->issue remains open
patch that I just checked in. I forgot to mention that I added tooltips to the navigation buttons in the minimonth and adapted the tooltips in the miniday to Christian's string recommendation.
Attachment #328640 - Flags: ui-review?(christian.jansen) → ui-review+
@Berend:  I checked this patch today in lightning 2008071020 and the Nav bar shows only 4 days. E.g. the week in the calendar view starts with Sunday -> the Nav bar shows 'Sunday, July 27, 2008 - Thursday, July 31, 2008. This bug is new, the *.xpi which I tested before (Comment #2) shows the correct entry (Sunday - Saturday)  
I also noticed something strange. After cvs upping included this patch, I had the app started and clicked on the "today" button. The 10th (which was today) was selected, but the view header told me I was in *June*. Clicking on "today" again gave me the correct month and I could not further reproduce.
Once more: Multiweek view, first day is July 6th, last day is August 16th -> Nav bar shows July 06 - August 14th. And in the month view only the navigation buttons are visible in the nav bar.
Obviously my final changes to meet Christian's comment #14 were not so unrisky as I thought, so I in this patch I drew them back.
>> Same question here? looks almost like gray, so maybe also a -moz color?
>No Christian insists to use his own color definitions. In this case the
>font-color has to be adapted to the image color.
I disputed this question with Christian some months ago. Christian was basically not happy with the limited offererd color definitions like "-moz-Field" -moz-Dialog" etc. and found they did not suffice his graphical demands. Therfor he accepted that his (hard coded) colors are equal over all themings. Although not being a graphical designer myself I understand that quite well. But in this special issue it makes even more sense to have the color of the font being equal to the color of the arrow images.
Attachment #329044 - Flags: review?(philipp)
Attachment #329044 - Flags: review?(philipp) → review+
checked in patch #3 on trunk and MOZILLA_1_8_BRANCH

Andreas tested the patch before I gave it to review.
Target Milestone: --- → 0.9
Attachment #328857 - Attachment description: revised patch #2 → [checked in] revised patch #2
Attachment #329044 - Attachment description: patch v. #3 → [checked in] patch v. #3
This checkin has caused the following strict warning:

  Warning: assignment to undeclared variable categories
  Source File: chrome://calendar/content/calendar-decorated-day-view.xml
  Line: 72
And the button icons are (still) missing in all four views using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.17pre) Gecko/2008071106 Calendar/0.9pre.
In month view please restore the useful header. I want to see with just one look whether it's "July" or "December". Currently it states "7/1/2008 - 7/31/2008" or "12/1/2008 - 12/31/2008".

Regression:
Especially when using the long date format this increases the minimum width required for the view very much. This means Sunbird/Lightning can't display correctly anymore on smaller screen resolutions. This regresses all bugs that have been fixed in the past to allow the view to be displayed on smaller screen resolutions.
This patch includes the missing file chrome://calendar/skin/widgets/calendar-widgets.css that define the button icons in Sunbird.
Attachment #329213 - Flags: review?(philipp)
Attachment #329213 - Flags: review?(philipp) → review+
Attachment #329213 - Attachment description: show button icons in Sunbird too → [checked in] show button icons in Sunbird too
(In reply to comment #25)
> In month view please restore the useful header. I want to see with just one
> look whether it's "July" or "December". Currently it states "7/1/2008 -
> 7/31/2008" or "12/1/2008 - 12/31/2008".
I agree, the month view should say the month name instead of the date.

> 
> Regression:
> Especially when using the long date format this increases the minimum width
> required for the view very much. This means Sunbird/Lightning can't display
> correctly anymore on smaller screen resolutions. This regresses all bugs that
> have been fixed in the past to allow the view to be displayed on smaller screen
> resolutions.
I'd suggest  to contract "Calendar Week" to "CW" when the window becomes small. Is this enough? Maybe the inverval could be wrapped into two lines when the window becomes small?

The Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.2pre) Gecko/2008071121 Calendar/0.6a1 nightly build is displaying the month and multiweek view correctly.
Note: in multi-week view, the weeks are arranged vertically (one, next, next, etc.) -- the buttons for next/prev, however, are horizontal. I wonder if there's a way to have vertically arranged buttons.
patch that introduces the new strings that Christian demanded in comment #9 for the date-formatted display
Attachment #329678 - Flags: review?(philipp)
Attachment #329678 - Flags: review?(philipp) → review+
Comment on attachment 329678 [details] [diff] [review]
[checked in] patch with new Strings

I think you need a real localization note for each property that clearly describes each parameter and that is recognized by the l10n tools.
<http://developer.mozilla.org/en/docs/Localization_notes>
Blocks: 322059
in reply to comment #31:
Thank you for that hint. Talked to Simon and will release a dev not in the localization board 'mozilla.dev.l10n'
I hope the localization notes in the patch attached give useful assistance to the localizers
Attachment #329793 - Flags: review?(daniel.boelzle)
Attachment #329678 - Attachment description: patch with new Strings → [checked in] patch with new Strings
Attachment #328640 - Attachment is obsolete: true
Attachment #329793 - Flags: review?(daniel.boelzle) → review+
Attachment #329793 - Attachment description: patch with localization notes → [checked in] patch with localization notes
"patch with localization notes" checked in on trunk and MOZILLA_1_8_BRANCH
The user gets no horizontal scroll bar in the calendar view since the new Nav Bar is implemented. 
patch with the new header with the formats that Christian had in mind in comment. I did not follow his suggestions for the day view:
> EN: March 3, 2008
> DE: 3. März 2008
In this case I took what the current standard dateformatter delivered which was slightly different.
I also eliminated the strict warning  that Stefan discovered in comment #22.
I also hope to to meet Andreas' and Stefan's complaints in comment #25 and comment #34

One implementation detail: I added a temporary function "formatNewInterval" to calIDateFormatter.idl and hope to replace the exsting "formatInterval" shortly after this patch has been integrated.
- the 
Before I am asking for review I would like to have this tested by somebody else, like Andreas.
Keywords: late-l10n
Comment on attachment 329844 [details] [diff] [review]
patch with new header format

Andreas tested the patch and found it was Ok. So I am asking for review.
@philipp: I hope that you can live with my implementation within the dateFormatter.js that is sort of "have finished". I rather want to complete this job within Bug 413103 shortly after.
Attachment #329844 - Flags: ui-review?(christian.jansen)
Attachment #329844 - Flags: review?(philipp)
Comment on attachment 329844 [details] [diff] [review]
patch with new header format

ui=christian

One small nit. It would be good if the day info could be formatted like specified in comment #4. However, this is not blocking.
Attachment #329844 - Flags: ui-review?(christian.jansen) → ui-review+
I've set Monday as first day of the week but in multiweek view the header show Sunday as first day (week from Sunday to Saturday instead of Monday to Sunday).
Lightning 0.9pre 2008071619
Comment on attachment 329844 [details] [diff] [review]
patch with new header format


>+            <property name="rangeStartDate">
>+              <setter><![CDATA[

>+                this.mRangeStartDate = val;
>+                return val;
If you like, this can be shortened to:
  return (this.mRangeStartDate = val);


r=philipp
Attachment #329844 - Flags: review?(philipp) → review+
patch checked in on trunk and MOZILLA_1_8_BRANCH.
-> fixed

I will continue with bug 413103 as promised in comment #36
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Depends on: 446366
The dates shown in the nav bar are still wrong for he multiweek view as pointed out in Comment #19 . I think it's a serious bug and I believe this should be fixed before 0.9 comes out. Worth blocking 0.9?
(In reply to comment #41)
> The dates shown in the nav bar are still wrong for he multiweek view as pointed
> out in Comment #19 . I think it's a serious bug and I believe this should be
> fixed before 0.9 comes out. Worth blocking 0.9?
> 

It's bug #446366 now.
Checked in nightly build 2008072219 ->VERIFIED (except the follow-up bug 446366)

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.