Closed Bug 430430 Opened 12 years ago Closed 12 years ago

Consolidate context menus

Categories

(Calendar :: Calendar Views, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: Fallen, Assigned: Fallen)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

While bug 340025 is meant to sort out which context menu items should be actually used and http://wiki.mozilla.org/Calendar:Context_Menu_Specification shows some context menus for the today pane, the current state of context menus differ between lightning and sunbird.

While fixing bug 429950, Sven reminded me of bug 413663. Wanting to integrate a fix for the latter, I ended up consolidating lots of context menu specific code. The two mentioned bugs are also fixed by this patch.

During ui-review, please keep in mind this patch should by no means sort out what context menu items we want, this can be done in a second step in bug 3440025.
Attached patch Consolidate context menus - v2 (obsolete) β€” β€” Splinter Review
Attachment #317233 - Flags: ui-review?(christian.jansen)
Attachment #317233 - Flags: review?(Berend.Cornelius)
Duplicate of this bug: 413663
Duplicate of this bug: 429950
Philipp, this is great stuff and exactly what I talked about on the Hamburg F2F regarding consolidating all app-independent code. Thanks a lot!
Flags: wanted-calendar0.9+
Comment on attachment 317233 [details] [diff] [review]
Consolidate context menus - v2

-<!ENTITY calendar.context.convert.label                "Convert to">
+<!ENTITY calendar.context.convertmenu.label             "Convert To">

Is there any need or reason to change to upcase here?
Attached patch Consolidate context menus - v3 (obsolete) β€” β€” Splinter Review
Update patch fixes some duplicate access keys and also adds missing keys (also consolidated)

Sven, the case change is needed to make the menus look good in the thread and message pane, where Copy To is also capital. See screenshot.
Attachment #317233 - Attachment is obsolete: true
Attachment #317264 - Flags: ui-review?
Attachment #317264 - Flags: review?(Berend.Cornelius)
Attachment #317233 - Flags: ui-review?(christian.jansen)
Attachment #317233 - Flags: review?(Berend.Cornelius)
Duplicate of this bug: 408650
Attachment #317264 - Flags: ui-review? → ui-review?(christian.jansen)
The menu looks good so far, please find my comments below. In general I'd like to see that the context menus move into the direction like specified in:

http://wiki.mozilla.org/Calendar:Context_Menu_Specification (Bug 340025)

But now to my comments:

Item Context Menu (Task)
 * Should offer the same content like "Task Context Menu"

Thread Pane Context Menu
 * Please move "Convert to >" below "Move to "Inbox" Again"

 * Is the missing separator (between "Move to "Inbox" Again" and Tag ) caused by the patch?
(In reply to comment #9)
> Item Context Menu (Task)
>  * Should offer the same content like "Task Context Menu"
Aside from this being a lot more work :-), this means that the task menu has no cut/copy/paste, but the item menu does. I'd like this to happen in another patch, since it requires re-modeling the way the task item is retrieved. 

> 
> Thread Pane Context Menu
>  * Please move "Convert to >" below "Move to "Inbox" Again"
Will fix after review.

> 
>  * Is the missing separator (between "Move to "Inbox" Again" and Tag ) caused
> by the patch?
No, its also that way without the patch

Attachment #317264 - Flags: ui-review?(christian.jansen) → ui-review+
Ok. ui+ r=christian
Patch basically looks and good works fine in both applications.
I noticed the following issues:
1) pasting an item also copies the start time. But it should be adapted to the mousepointer position over the grid and the selected day.
-I see that you have four different but similar implementations of the "context-menu-convert-menu" where you partly control the type by means of the "setupContextItemType" function and css attributes. Wouldn't it be better and worth it to create an own binding for this?

2) your function "setupContextItemType": What happens if you have several items of a different type selected ("event" or task"). To retrieve the type from the first selected Item is wrong in this case, isn't it? I find you have to disable (my preference) or hide all the type relevant menuitems.

3) You did not consolidate the "calendar-new-event-key" and the "calendar-new-event-key" obviously because of the different "key" attributes in Sunbird and Lightning. Wouldn't it be better to consolidate the keys and only define two different application-dependent resources?

4) From a User Interface point of view I don't like the "Edit Item" and "Delete Item" labels. I recently talked with Christian about it and we found that "Edit" and "Delete" (with "..."?) are better because less technical. 

I wouldn't mind if these issues are dealt with in a follow up bug.
Comment on attachment 317264 [details] [diff] [review]
Consolidate context menus - v3

r=berend. Thank you for your contribution
Attachment #317264 - Flags: review?(Berend.Cornelius) → review+
(In reply to comment #12)
> 4) From a User Interface point of view I don't like the "Edit Item"
> and "Delete Item" labels.

Bug 374464 for Lightning, similar Bug 369934 for Sunbird.
Blocks: 431390
(In reply to comment #12)
> 1) pasting an item also copies the start time. But it should be adapted to the
> mousepointer position over the grid and the selected day.
bug 431390

> -I see that you have four different but similar implementations of the
> "context-menu-convert-menu" where you partly control the type by means of the
> "setupContextItemType" function and css attributes. Wouldn't it be better and
> worth it to create an own binding for this?
Also bug 431390

> 
> 2) your function "setupContextItemType": What happens if you have several items
> of a different type selected ("event" or task"). To retrieve the type from the
> first selected Item is wrong in this case, isn't it? I find you have to disable
> (my preference) or hide all the type relevant menuitems.
Will fix.

> 
> 3) You did not consolidate the "calendar-new-event-key" and the
> "calendar-new-event-key" obviously because of the different "key" attributes in
> Sunbird and Lightning. Wouldn't it be better to consolidate the keys and only
> define two different application-dependent resources?
Not possible, bug 290747

> 
> 4) From a User Interface point of view I don't like the "Edit Item" and "Delete
> Item" labels. I recently talked with Christian about it and we found that
> "Edit" and "Delete" (with "..."?) are better because less technical. 
> 
> I wouldn't mind if these issues are dealt with in a follow up bug.
> 
As ssitter noted
Patch with comments fixed, carrying forth r+
Attachment #317264 - Attachment is obsolete: true
Attachment #318472 - Flags: ui-review+
Attachment #318472 - Flags: review+
Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.9
Checked in lightning 2008082103 and sunbird 20080820 -> VERIFIED
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.