Unify/cleanup navigation menu in all modes (Mail/Calendar/Task)

VERIFIED FIXED in 0.8

Status

VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: giermann, Assigned: giermann)

Tracking

unspecified

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

11 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.1.11) Gecko/20071127 Firefox/2.0.0.11
Build Identifier: Lightning 0.8pre (build 2008012119)

The Navigation menu should be unique in all 3 modes to switch between these modes and to view the address book.


Reproducible: Always

Actual Results:  
I found the following:

Mail mode:
- no menu to switch to Calendar/Task mode (only in Mode toolbar)
- Addressbook in "Tools"

Calendar mode:
- Mail/Calendar/Task switching in menu "Navigation"
- Addressbook both in "Navigation" and "Tools"

Task mode:
- Mail/Calendar/Task switching in menu "Navigation"
- Addressbook only in "Tools"


Expected Results:  
Find a unique place to switch between modes and to display Addressbook and leave these at the same position for all modes.

You may recognize that this is some kind of spin-off bug for attachment #280382 [details] [diff] [review] (bug #386479).
(Assignee)

Updated

11 years ago
Flags: wanted-calendar0.8?
OS: Windows XP → All
Hardware: PC → All

Updated

11 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 1

11 years ago
Created attachment 298938 [details]
Cleanup patch for bug #408798

I tried to clean up the code of attachment #297819 [details] [diff] [review].

I decided to display "Addressbook" both in "Go" and "Tools" as suggested by Christian in [1].

@Berend: Could you please have a look at this? Is there any reason you stored the menus in 'copyPopupMenus()' into 'menulist' but did not use them for creating the calendar/task menu popups?
I had also to remove the FIRST menuseparators in 'removeLastMenuSeparator()' to clean up the "Go" menu.

[1] http://spreadsheets.google.com/pub?key=plCAueWeXt4hSxoRkxWFJCQ
Attachment #298938 - Flags: review?(Berend.Cornelius)
(Assignee)

Comment 2

11 years ago
Created attachment 298939 [details] [diff] [review]
Cleanup patch for bug #408798

I tried to clean up the code of attachment #297819 [details] [diff] [review].

I decided to display "Addressbook" both in "Go" and "Tools" as suggested by Christian in [1].

@Berend: Could you please have a look at this? Is there any reason you stored the menus in 'copyPopupMenus()' into 'menulist' but did not use them for creating the calendar/task menu popups?
I had also to remove the FIRST menuseparators in 'removeLastMenuSeparator()' to clean up the "Go" menu.

[1] http://spreadsheets.google.com/pub?key=plCAueWeXt4hSxoRkxWFJCQ
Attachment #298939 - Flags: review?(Berend.Cornelius)
(Assignee)

Updated

11 years ago
Attachment #298938 - Attachment is obsolete: true
Attachment #298938 - Flags: review?(Berend.Cornelius)
(Assignee)

Comment 3

11 years ago
Sorry for the spam, I did not get any response to the first try of attaching the patch...
Assignee: nobody → giermann
Status: NEW → ASSIGNED

Comment 4

11 years ago
Your patch works great and leaves nothing to complain.
>+    ltnRemoveMailOnlyItems(calendarpopuplist, "calendar");
>+    ltnRemoveMailOnlyItems(taskpopuplist, "task");

Good point to consolidate this!

>Is there any reason you stored
>the menus in 'copyPopupMenus()' into 'menulist' but did not use them for
>creating the calendar/task menu popups?

-I found the code more self explaining with
document.getElementById("blablaMenu").
Anyway I like your version better now. The best thing would probably be to use
constants instead of literals for the array-index.

-The function-name "removeLastMenuSeparator" does not really fit anymore. How
about "removeNeedlessSeparators"?

Updated

11 years ago
Attachment #298939 - Flags: review?(Berend.Cornelius) → review+
(Assignee)

Comment 5

11 years ago
Created attachment 299696 [details] [diff] [review]
Patch with the suggestion in comment #4 applied.

Leaving r=Berend.Corenelius, as only the name of the function changed.

-- ready for check-in --
Attachment #298939 - Attachment is obsolete: true
Attachment #299696 - Flags: review+
Keywords: checkin-needed
Target Milestone: --- → 0.8

Comment 6

11 years ago
patch checked in on trunk and MOZILLA_1_8_BRANCH.
=> issue is fixed
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Flags: wanted-calendar0.8?
Keywords: checkin-needed

Comment 7

11 years ago
Verified with Lightning 2008012900
Status: RESOLVED → VERIFIED
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.9) Gecko/20071031 Lightning/0.8pre (2008012818) Thunderbird/2.0.0.9 ID:2007103104

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