Closed Bug 363932 Opened 17 years ago Closed 15 years ago

Delete Warning should be redesigned

Categories

(Calendar :: Calendar Frontend, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

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

References

(Blocks 1 open bug)

Details

Attachments

(7 files, 3 obsolete files)

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: nobody → philipp
Blocks: 320178
No longer depends on: 320178
Version: Trunk → unspecified
Attached patch Fix v1 (obsolete) — Splinter Review
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)
Attached image Screenshot linux
Attachment #320033 - Flags: ui-review?(christian.jansen)
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
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]
----------------
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

()
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
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 on attachment 320032 [details] [diff] [review]
Fix v1

r=berend
Attachment #320032 - Flags: review?(Berend.Cornelius) → review+
(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.
>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.
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.
Christian, whats your decision about the button height? How many px? Also, do you have ETA on the images?
(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.
> 

(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.
Attached patch Fix localization issue - v1 (obsolete) — Splinter Review
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)
Attached patch Fix localization issue - v1 (obsolete) — Splinter Review
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+
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+
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
Attached patch Add icons codeSplinter Review
If anyone would like to steal review, please go ahead :-)
Attachment #333308 - Flags: review?(daniel.boelzle)
Comment on attachment 333308 [details] [diff] [review]
Add icons code

looks good, r=mschroeder
Attachment #333308 - Flags: review?(daniel.boelzle) → review+
Finally, all parts are taken care of...


Checked in on HEAD and MOZILLA_1_8_BRANCH

-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
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.