Delete Warning should be redesigned

RESOLVED FIXED in 0.9

Status

RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: chris.j.bugzilla, Assigned: Fallen)

Tracking

(Blocks: 1 bug)

unspecified

Details

Attachments

(7 attachments, 3 obsolete attachments)

(Reporter)

Description

12 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.1) Gecko/20061010 Firefox/2.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.1) Gecko/20061010 Firefox/2.0

The current warning, which appear if user delete one event of a series, is not easy to understand. It should be redesigned like proposed in the wiki.

http://wiki.mozilla.org/Calendar:Alerts#Deleting_an_Event

Reproducible: Always

Steps to Reproduce:
1. Create a repeating event
2. Select one event of the series
3. Hit DEL
The discussion on this topic was originally initiated to find a solution for Bug 320178. Close Bug 320178 and implement the UI proposal with this new bug or mark dependency or ...? As pointed out in the other bug support for 'all future' needs to be added to the calendar backend first.
Whiteboard: [qa discussion needed]
Status: UNCONFIRMED → NEW
Depends on: 320178
Ever confirmed: true
Whiteboard: [qa discussion needed]
Version: unspecified → Trunk
(Assignee)

Updated

11 years ago
Assignee: nobody → philipp
Blocks: 320178
No longer depends on: 320178
Version: Trunk → unspecified
(Assignee)

Comment 2

11 years ago
Created attachment 320032 [details] [diff] [review]
Fix v1

I would like to get the prerequisites in first, therefore I switched the blocks/depends order. While this dialog needs the backend functionality, the backend functionality is quite tricky. The patch size is dramatically lowered this way, I just need to implement the actual item splitting.

Since there are no icons yet, the pixel numbers are obviously wrong. I might have to check this patch on other OS, since it will obviously look different. IIRC, the wizard on windows has a white header. The same color is used in this header, but on linux its all gray.

Also, the this and future button should be hidden. I just left it in for effects. Will post a screenshot soon.
Attachment #320032 - Flags: review?(Berend.Cornelius)
(Assignee)

Comment 3

11 years ago
Created attachment 320033 [details]
Screenshot linux
Attachment #320033 - Flags: ui-review?(christian.jansen)
(Assignee)

Comment 4

11 years ago
Also there is a Problem with events with long titles. Not quite sure how to solve this: Since the whole phrase is one label, I can only crop at start/end/center, but not just before the quotes.

Berend, if you have an idea, please tell me about it :-)
Status: NEW → ASSIGNED
(Reporter)

Comment 5

11 years ago
I can imagine that two left aligned rows would work fine.
One for the title, the other for the description.

"Event Title"
Is a repeating event
----------------
[Button 1]
[Button 2]
[Button 3]
----------------
(Reporter)

Comment 6

11 years ago
Comment on attachment 320033 [details]
Screenshot linux

The default button should be focused initially (in this case "Edit just this occurrence").

It might also be OK to have different button heights this would help users to focus the "most" important one (see proposal attached).

Please remove the icons until I (or anyone who wants to volunteer ;-) ) has created a set.

We need icons for:

Edit
Delete
Move
Copy

()
(Reporter)

Comment 7

11 years ago
Created attachment 320862 [details]
mock-up with different button heights
(Assignee)

Comment 8

11 years ago
Setting uiwanted to get UI.

What do you mean with move and copy? I haven't implemented anything for those actions, and move is just a special form of edit.

Do you have a suggested height for the button? I'm not sure the proposed height is sufficient, it didn't really catch my eye, but otoh maybe its subliminal :-)

I'll adapt the pixel heights and split the labels after code review.
Keywords: uiwanted

Comment 9

11 years ago
The patch works as advertised: Some remarks:
-As the button "Edit this and all future occurrences" has not yet any implemented  action behind we should probably rather disable it for the time being or maybe raise a messagebox.
- Your implementation "promptOccurrenceModification()" is not really easy to understand: 
- I would call the third parameter "aAction" instead of "aType". I don't know if I like your way to map its value directly to the resource id - but it works fine. What could happen is that somebody who wants to tidy up the resources might think that e.g the resource "buttons.occurrence.delete/edit.label" is not used (because its identifier is concatenated in the source code) and may be deleted.
- In your returned array "futureItem" can either be null or an item or a boolean that  is meant to be dependent on the second parameter "aNeedsFuture" when this is "false". The latter condition has not yet been implemented. I would try to make this easier.
- I am asking myself if your local constants
>    const CANCEL = 0;
>    const MODIFY_OCCURRENCE = 1;
>    const MODIFY_FOLLOWING = 2;
>    const MODIFY_PARENT = 3;
should not be defined in an interface and probably passed as an inout parameter instead of passing "aNeedsFuture", but this is just an idea that you probably also have thought about already...

Comment 10

11 years ago
Comment on attachment 320032 [details] [diff] [review]
Fix v1

r=berend
Attachment #320032 - Flags: review?(Berend.Cornelius) → review+
(Assignee)

Comment 11

11 years ago
(In reply to comment #9)
> The patch works as advertised: Some remarks:
> -As the button "Edit this and all future occurrences" has not yet any
> implemented  action behind we should probably rather disable it for the time
> being or maybe raise a messagebox.
I chose to hide it for now.

> - Your implementation "promptOccurrenceModification()" is not really easy to
> understand: 
> - I would call the third parameter "aAction" instead of "aType". I don't know
> if I like your way to map its value directly to the resource id - but it works
> fine. What could happen is that somebody who wants to tidy up the resources
> might think that e.g the resource "buttons.occurrence.delete/edit.label" is not
> used (because its identifier is concatenated in the source code) and may be
> deleted.
I renamed the parameter. I see your concern, we have been doing this all along though (i.e calGetString("recurrenceDetailsDay" + day_of_week(byday[0])) and I think if someone wants to clean up the properties (bug 434985), then he has to do some more research than just to grep for the full string.

> - In your returned array "futureItem" can either be null or an item or a
> boolean that  is meant to be dependent on the second parameter "aNeedsFuture"
> when this is "false". The latter condition has not yet been implemented. I
> would try to make this easier.
I could add a third return array parameter with the type and then make that be one of the constants, how's that?

> - I am asking myself if your local constants
> >    const CANCEL = 0;
> >    const MODIFY_OCCURRENCE = 1;
> >    const MODIFY_FOLLOWING = 2;
> >    const MODIFY_PARENT = 3;
> should not be defined in an interface and probably passed as an inout parameter
> instead of passing "aNeedsFuture", but this is just an idea that you probably
> also have thought about already...
aNeedsFuture is more for places like delete all following, where the future item is not needed. This has nothing to do with the returned constants.

I think an interface for UI code is not needed.

Comment 12

11 years ago
>I could add a third return array parameter with the type and then make that be
>one of the constants, how's that?
Yes that's better because more understandable.
(Assignee)

Comment 13

11 years ago
Created attachment 321943 [details] [diff] [review]
[checked in] Fix - v3

I've checked in this patch since I need it for a future bug, but leaving this bug open to check in the images needed for the dialog and to adjust the button height if needed.
Attachment #320032 - Attachment is obsolete: true
Attachment #321943 - Flags: ui-review+
Attachment #321943 - Flags: review+
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.15pre) Gecko/2008052110 Calendar/0.9pre

With this new added dialog the wording is completely wrong when trying to edit a repeating task. 

The previous dialog used the wording "repeating item". The new dialog always uses the wording "repeating event". That is just wrong for tasks.

The dialog doesn't seem to be resizeable and the dimensions seem to be hard coded in css. This might cause issues with some translations.
(Assignee)

Comment 15

10 years ago
Christian, whats your decision about the button height? How many px? Also, do you have ETA on the images?
(Reporter)

Comment 16

10 years ago
(In reply to comment #14)
> Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.15pre)
> Gecko/2008052110 Calendar/0.9pre
> 
> With this new added dialog the wording is completely wrong when trying to edit
> a repeating task. 
> 
> The previous dialog used the wording "repeating item". The new dialog always
> uses the wording "repeating event". That is just wrong for tasks.

That true. We need to differentiate here. I can't recommend to use item, because it is a very abstract, and technical description.
> 
> The dialog doesn't seem to be resizeable and the dimensions seem to be hard
> coded in css. This might cause issues with some translations.
> 

(Reporter)

Comment 17

10 years ago
(In reply to comment #15)
> Christian, whats your decision about the button height? How many px? Also, do
> you have ETA on the images?
> 

I'll work on the images beginning next week.
(Assignee)

Comment 18

10 years ago
Created attachment 328739 [details] [diff] [review]
Fix localization issue - v1

Since the string freeze is on monday, I'll fix the localization issue extra. It would be nice to get some icons, or at least the requested button heights.
Attachment #328739 - Flags: review?(mschroeder)
(Assignee)

Comment 19

10 years ago
Created attachment 328740 [details] [diff] [review]
Fix localization issue - v1

Argh, I forgot to save before uploading.
Attachment #328739 - Attachment is obsolete: true
Attachment #328740 - Flags: review?(mschroeder)
Attachment #328739 - Flags: review?(mschroeder)
Comment on attachment 328740 [details] [diff] [review]
Fix localization issue - v1

r=mschroeder, but you should adjust the comments (header is mentioned twice).

>       // Set up title and header
>       document.title =
>         calGetString("calendar-occurrence-prompt",
>-                     "windowtitle." + action);
>+                     "windowtitle." + itemType + "." + action);
>       document.getElementById("title-label").value = window.arguments[0].item.title;
> 
>+      // Set up header
>+      document.getElementById("isrepeating-label").value =
>+        calGetString("calendar-occurrence-prompt",
>+                     "header.isrepeating." + itemType + ".label");
>+
Attachment #328740 - Flags: review?(mschroeder) → review+
(Reporter)

Comment 21

10 years ago
Comment on attachment 320033 [details]
Screenshot linux

Please make "Edit just this occurrence" the default button. The size of the icons will be 20x20px. Please also align the heading with the left button edge.
ui=christian
Attachment #320033 - Flags: ui-review?(christian.jansen) → ui-review+
(Assignee)

Comment 22

10 years ago
Created attachment 328935 [details] [diff] [review]
[checked in] Fix localization issue - v2

Patch as checked in. I already prepared the CSS files for the 20x20 px images too.
Attachment #328740 - Attachment is obsolete: true
Attachment #328935 - Flags: ui-review+
Attachment #328935 - Flags: review+
Target Milestone: --- → 0.9
(Assignee)

Comment 23

10 years ago
Created attachment 333308 [details] [diff] [review]
Add icons code

If anyone would like to steal review, please go ahead :-)
Attachment #333308 - Flags: review?(daniel.boelzle)
(Assignee)

Comment 24

10 years ago
Created attachment 333309 [details]
Winstripe Icons (Windows,Linux)
(Assignee)

Comment 25

10 years ago
Created attachment 333310 [details]
Pinstripe Icons (Mac)
Comment on attachment 333308 [details] [diff] [review]
Add icons code

looks good, r=mschroeder
Attachment #333308 - Flags: review?(daniel.boelzle) → review+
(Assignee)

Comment 27

10 years ago
Finally, all parts are taken care of...


Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Keywords: uiwanted
The space between the icon and the button's label looks quite close to me; Christian suggested to insert 3px like we do elsewhere, too.
(In reply to comment #28)

I filed Bug 450611 to follow-up the issue.
You need to log in before you can comment on or make changes to this bug.