Closed Bug 454933 Opened 12 years ago Closed 11 years ago

[calendar integration] move month, day, week mode buttons into calendar view

Categories

(Calendar :: Lightning Only, defect, P1)

defect

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: clarkbw, Assigned: berend.cornelius09)

References

()

Details

Attachments

(3 files, 3 obsolete files)

Part of the calendar integration lightning needs to move the mode buttons for changing between day, week, and month views into the calendar view itself.  This change is required for bug 454931, removing the toolbar.  Description and mockups in the wiki page link provided.
whoops, copy and paste mistake.  Bug 454935 is the remove toolbar bug.
Blocks: 454935
Flags: tb-integration?
Duplicate of this bug: 456352
Christian, could you please clarify how this affects Sunbird? Should the toolbar icons be removed for Sunbird, too?
Flags: tb-integration? → tb-integration+
Flags: blocking-calendar1.0+
Assignee: nobody → Berend.Cornelius
Attached patch patch v. #1Splinter Review
My plan was to implement the calendar views within a tabbox container and the existing toolbarbuttons replaced by tabs. Although I still think this is the way it should go I again made the experience that it can be quite dangerous to refactor existing implementations in one step. The patch attach does not really work but I want to keep it for probable later use.
I will start a new approach and replace the toolbarbuttons by "tab" elements in a "tabs" container and leave the basic code structure as it is.
Will you please consider giving us four css IDs that we can reference from userchrome.css for each of the day/week/month/multiweek tabs?  I never use day or week views and it would be nice if I had the choice to remove the day/week tabs from the view (using "display: none").
Priority: -- → P1
Attached patch patch v. #2 (obsolete) — Splinter Review
newest patch that still has some ui issues to be discussed over
Berend, the patch misses the theme changes in base/themes/pinstripe. Was that intentional?
Yes it is. I will add the pinstripe code after I resolved the open issues to save me duplicate work and testing.
Attached image screenshot
I attached a screenshot with the current state of the art and discussed it over with Christian. As expected we agreed to do some modifications:
- during "onMouseover" of the tabs they should adapt to another darker grey background. The active tab should be white the others grey.
-There should be an additional margin set at the top of the line and at the bottom- each 2-3px. The minimonth should be lowered accordingly. In this respect there is no need to consider the Unifinder splitter because this is bound to be skipped from the product.
-For the calendar week string we should use the short form "CW". We should not make it bold.
-The tabs should not jump after a reselection and when switching from Month to day-view.
Does this work on smaller screen resolutions too? I think this might make working impossible due to the increased width. Especially if the translation for e.g. day/week/multiweek/month or the date string take up additional characters in some localizations.
Status: NEW → ASSIGNED
Hardware: PC → All
(In reply to comment #10)
> Does this work on smaller screen resolutions too? I think this might make
> working impossible due to the increased width. Especially if the translation
> for e.g. day/week/multiweek/month or the date string take up additional
> characters in some localizations.

...and with today pane open as well.
Attached patch patch v. #3 (obsolete) — Splinter Review
This patch takes care of the issues requested by Christian. 
Some things that remain to be resolved:
-The whole architecture of the navigation panel must be overworked. Currently we have one navigation panel for each view which is currently suboptimal. There should be one navigation panel for all calendar views.
- I failed to prevent the tabs from "jumping" after selection because of the bold label of the selected tab. There must be way to do this, but I hope it's Ok if we deal with it in a follow-up bug (unless Philipp has another idea how to do this).
- the focus rectangle in the tabs control is set in a weird way. I would like to deal with this problem later on, too. In fact for whatever the reason may be the tabs control is not really accessible.
-Decathlon's and Stefan's concerns about a too small navigation pane in conjunction with small desktop resolution and open Todaypane can be met with adding an overflow - behaviour to the labels, so that these only display a shortform description when an overflow occurs. I experimented a bit with this and hope to deliver a patch in the coming time. I can see their point but I personally see no real alternative to the basics of the user interface of this new panel.
Attachment #345108 - Attachment is obsolete: true
Attachment #348219 - Flags: ui-review?(christian.jansen)
Attachment #348219 - Flags: review?(philipp)
Comment on attachment 348219 [details] [diff] [review]
patch v. #3

>+                    // The following code is about preventing the tabs from expanding when being selected
>+                    // due to their bold label. I currently does not work, but we should maybe keep it until
>+                    // we solved the problem
Instead I'd like to go with a locale-specific solution for now. See attached patch.

>+/* Navigation controls for the views */
>+calendar-navigation-buttons {
>+    background-color: white;
>+}
White background? I assume there is no way we can use system colors? Maybe you can find a solution with Christian for this.

>+}
>\ No newline at end of file
Please, make your editor insert newlines at the end of files!! I've changed this in the patch I'll upload.


I've done some other minor changes and implemented the locale-specific rules. I'll attach a patch you can fix the background issue on if needed.

Aside from that, I really don't like the way the navigation buttons ( former "< o >" ) look like. More specifically, the hover seems a bit strange and "Today" doesn't really look like a button until hovered. It may be a bit confusing, i.e user thinks "Today" is in the view (day view, most notably). But I guess this is up to Christian.


r=philipp with changes made.
Attachment #348219 - Flags: review?(philipp) → review+
Attached patch patch v. #4 (obsolete) — Splinter Review
Attachment #348219 - Attachment is obsolete: true
Attachment #348472 - Flags: ui-review?(christian.jansen)
Attachment #348219 - Flags: ui-review?(christian.jansen)
Comment on attachment 348472 [details] [diff] [review]
patch v. #4

Hi Philipp,

two flyby notes to your patch.

>diff --git a/calendar/locales/en-US/chrome/calendar/calendar.dtd b/calendar/locales/en-US/chrome/calendar/calendar.dtd
>--- a/calendar/locales/en-US/chrome/calendar/calendar.dtd
>+++ b/calendar/locales/en-US/chrome/calendar/calendar.dtd
>@@ -130,6 +130,11 @@
> <!ENTITY calendar.displaytodos.checkbox.accesskey "k" >
> <!ENTITY calendar.completedtasks.checkbox.label     "Show completed Tasks" >
> <!ENTITY calendar.completedtasks.checkbox.accesskey "c" >
>+
>+<!ENTITY calendar.day.tab.minwidth            "3em" >
>+<!ENTITY calendar.week.tab.minwidth           "4em" >
>+<!ENTITY calendar.month.tab.minwidth          "5em" >
>+<!ENTITY calendar.multiweek.tab.minwidth      "7em" >

Please add a localization note here for localizers telling them, that the "em" should not be localized and are used for the individual width of the respective tabs.
 
>diff --git a/calendar/locales/en-US/chrome/calendar/calendar.properties b/calendar/locales/en-US/chrome/calendar/calendar.properties
>--- a/calendar/locales/en-US/chrome/calendar/calendar.properties
>+++ b/calendar/locales/en-US/chrome/calendar/calendar.properties
>@@ -367,6 +367,9 @@
> longCalendarWeek=Calendar Week: %1$S
> severalLongCalendarWeeks=Calendar Weeks %1$S-%2$S
> 
>+shortCalendarWeek=CW: %1$S
>+severalShortCalendarWeeks=CWs: %1$S-%2$S

Please add a localization note here as well, detailing what %1$S and %2$S mean here and in the two strings directly above the two new strings. I would also really appreciate it, if you could document the other instances of placeholders in this file, either right now or in a followup bug.
Blocks: 465317
Blocks: 465319
Blocks: 465321
Attachment #348472 - Flags: ui-review?(christian.jansen) → ui-review+
Comment on attachment 348472 [details] [diff] [review]
patch v. #4

I've filed a bunch of follow-up bugs. For finalizing the new ui introduced by this patch. I'm aware that this patch does not deliver the final look and feel, but with fixing the bugs 465321, 465319, 465317 the overall appearance should be in good shape.

ui+=christian
Attached patch patch v. #5Splinter Review
I added localization notes as requested by Simon in comment #15.
After a discussion with Philipp and Christian (Simon was not available by that time) we decided to slightly change some of Philipp's modifications in his patch #3: The width of the tab buttons is now calculated in the constructor of the navigationpanel according to the length of the label, so that the localizers are out of the game now. This makes the tabs in some places probably a bit too large but on the other hand it is also less error prone when the localizers make  a mistake here.
patch v. #5 pushed to comm-central:
http://hg.mozilla.org/comm-central/rev/7985bc96df1e

issues is fixed with consideration to the follow-up bugs posted by Christian.
Attachment #348472 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.0
We have shortcalendarweek and shortCalendarWeek in the same file. Maybe one of these keys should be changed to avoid confusion...

shortcalendarweek=CW
shortCalendarWeek=CW: %1$S
Blocks: 466131
(In reply to comment #18)
> We have shortcalendarweek and shortCalendarWeek in the same file. Maybe one of
> these keys should be changed to avoid confusion...
> 
> shortcalendarweek=CW
> shortCalendarWeek=CW: %1$S

Hubert, you are right, but please file separate bug reports for follow-up issues. :)
Checked in lightning build 20081130 -> VERIFIED.
Status: RESOLVED → VERIFIED
These bugs are likely targeted at Lightning 1.0b1, not Lightning 1.0. If this change was done in error, please adjust the target milestone to its correct value. To filter on this bugspam, you can use "lightning-10-target-move".
Target Milestone: 1.0 → 1.0b1
You need to log in before you can comment on or make changes to this bug.