Closed
Bug 378270
Opened 17 years ago
Closed 17 years ago
Remove 'Rotate' button and move the rotate option to the views menu
Categories
(Calendar :: Calendar Frontend, defect)
Calendar
Calendar Frontend
Tracking
(Not tracked)
VERIFIED
FIXED
0.7
People
(Reporter: sipaq, Assigned: Fallen)
Details
Attachments
(3 files, 4 obsolete files)
77.43 KB,
image/png
|
Details | |
23.48 KB,
patch
|
michael.buettner
:
review+
|
Details | Diff | Splinter Review |
1.03 KB,
patch
|
michael.buettner
:
review+
|
Details | Diff | Splinter Review |
In the discussion on the Hamburg F2F meeting, it was decided, that the current 'Rotate' button is bad UI and should be removed. The option to rotate the view should be moved to the 'views' menu instead.
Reporter | ||
Comment 1•17 years ago
|
||
These are the relevant places in the code, that need to be changed: - mozilla/calendar/locales/en-US/chrome/calendar/calendar.properties, line 208 - mozilla/calendar/base/content/calendar-multiday-view.xml, line 1783 - mozilla/calendar/base/content/calendar-multiday-view.xml, line 1817 - mozilla/calendar/base/content/calendar-multiday-view.xml, line 1818
Comment 2•17 years ago
|
||
Please state the reasons for your decisions. Otherwise, this bug is either invalid or wontfix.
Reporter | ||
Comment 3•17 years ago
|
||
The reasons as per bug 358692 comment 13: - In general it makes sense to have a "Rotate" option, but we should not display this feature top-level. - The button consumes much space and is located at a very prominent place. The calendar layout should be as clean as possible. - Moving the Rotate button into the "View" or "Calendar" menu should be ok. We could also offer a toolbar item, but this should be turned off by default. - View-related settings (e.g. number of weeks in multiweeks, workweek days only, etc.) should be in the views menu, not spread around the UI.
Assignee | ||
Comment 4•17 years ago
|
||
Milestone 0.7 Consolidation. Filter on PinkyANDBrain to get rid of bugspam.
Target Milestone: Lightning 0.7 → Sunbird 0.7
Assignee | ||
Comment 5•17 years ago
|
||
Mickey, how far are you on this? I hope its ok that I'm taking this one, but I only saw that the bug was assigned to you after creating the patch. Note it only adds the menu items correctly for Sunbird at the moment. I would like some input on how best to solve this for lightning, since the calendar/view menu is quite different there. Requesting ui review for that reason.
Assignee: michael.buettner → bugzilla
Status: NEW → ASSIGNED
Attachment #263654 -
Flags: ui-review?(christian.jansen)
Attachment #263654 -
Flags: review?(michael.buettner)
Assignee | ||
Comment 6•17 years ago
|
||
Menu only enabled in multiday views.
Comment 7•17 years ago
|
||
Drive by comment: In my opinion this two items doesn't justify a sub menu and bloat the UI. I think a simple menu item (e.g. Horizontal Orientation) with on/off state like the "Task in View" menu item below would be better. This also reduces the required mouse action to change the orientation. I also think the user decision about the view orientation / menu item state should be persistent between view switches and application restarts (similar to the "Task in View" item again).
Comment 8•17 years ago
|
||
Just some more random thoughts about the wording that might be more descriptive: "Arrange as Timescale" or "View as Timescale" or "Arrange View by Date" vs. "Arrange View by Time" or "Orient View by Date" vs. "Orient View by Time" or ...
Comment 9•17 years ago
|
||
I agree to Stefan. Having one entry with a checkbox should be enough. ... ------------------- Workweek days only Tasks in View Number of Weeks > [ ] Rotate View ------------------- ... Regarding the string: From my point of view the string "Rotate View" would be somehow self-explaining.
Comment 10•17 years ago
|
||
I agree to Stefan. Having one entry with a checkbox should be enough. ... ------------------- Workweek days only Tasks in View Number of Weeks > [ ] Rotate View ------------------- ... Regarding the string: From my point of view the string "Rotate View" would be somehow self-explaining.
Comment 11•17 years ago
|
||
The checkbox solution would also work better for Lightning There we would have Calendar -> View -> Workweek days only Tasks in View [ ] Rotate View
Assignee | ||
Comment 12•17 years ago
|
||
Ok, this should do it, both for sunbird and lightning.
Attachment #263654 -
Attachment is obsolete: true
Attachment #263745 -
Flags: ui-review?(christian.jansen)
Attachment #263745 -
Flags: review?(michael.buettner)
Attachment #263654 -
Flags: ui-review?(christian.jansen)
Attachment #263654 -
Flags: review?(michael.buettner)
Assignee | ||
Comment 13•17 years ago
|
||
Screenshot, also includes lightning UI in various modes. Horizontal (Rotated) view is always enabled here.
Attachment #263655 -
Attachment is obsolete: true
Comment 14•17 years ago
|
||
Looks good to me. Thanks for submitting the patch. A small side note: It is really confusing to see a disabled menu item, with an enabled checkbox. But I guess that this needs to be fixed somewhere else.
Updated•17 years ago
|
Attachment #263745 -
Flags: ui-review?(christian.jansen) → ui-review+
Assignee | ||
Comment 15•17 years ago
|
||
Yes, I was thinking the same thing, but disabling the checkbox manually will make problems when you go into month view and then quit the application. The persist property will keep the item from being checked after restart, and the multiday views will start in vertical mode every startup.
Comment 16•17 years ago
|
||
Sorry for jumping in here late, but why exists a calendar/view menu while there's a top-level view menu in thunderbird? Shouldn't we put all the calendar-view related options under view/calendar? Of course this has basically nothing to do with this patch, but it could be worth a spin-off bug? Christian, any comments?
Comment 17•17 years ago
|
||
Thanks for jumping in :-) You are right we have to define that. We should create one spin-off bug for the whole calendar menu restructuring. With this issue we move the rotate to entry to its final position.
Comment 18•17 years ago
|
||
Comment on attachment 263745 [details] [diff] [review] Remove rotate button and put in menu > /** >+ * The orientation of the view. Common values are "horizontal" and "vertical" >+ */ >+ attribute AUTF8String orient; Wouldn't it be better to make this a boolean attribute? A string can be set to anything and is generally less restrictive than a boolean switch. Or do we need to take any other layout options into account? Phillip, I'm not strongly against a string here, but is there any particular reason for not making it a boolean? Furthermore, it feels strange to move options that are specific to a particular type of view to the most general interface. But as far as I see, we don't have any other option here. At least we'd have the theoretical option to construct a horizontal month view :-) Besides what I mentioned above, it bothers me that the "rotate view" option gets disabled but the checkbox does not. Wouldn't it be much better to not disable the menu entry at all? Is it wrong to toggle the option while not being in the week view? At least, the "workweek days only" and "tasks in view" options also don't get disabled while not being in the calendar. Christian, any comments on this? Otherwise, I'd prefer to not disable the option at all.
Comment 19•17 years ago
|
||
I use 125% sized fonts in Windows, which makes the "Rotate" button too wide, so that the cells in week view next to the button are narrower than the columns below them, which looks ugly. Before it is decided what to do with the menu, a limit on the size of the button would be helpful. I don't want to file it as a new bug but don't know where to send the snapshot.
Assignee | ||
Comment 20•17 years ago
|
||
(In reply to comment #18) > (From update of attachment 263745 [details] [diff] [review]) > > /** > >+ * The orientation of the view. Common values are "horizontal" and "vertical" > >+ */ > >+ attribute AUTF8String orient; > Wouldn't it be better to make this a boolean attribute? A string can be set to > anything and is generally less restrictive than a boolean switch. Or do we need > to take any other layout options into account? Phillip, I'm not strongly > against a string here, but is there any particular reason for not making it a > boolean? I changed to a boolean, at least in calIDecoratedView. It sets either horizontal or vertical in the underlying calendar view. > > Furthermore, it feels strange to move options that are specific to a particular > type of view to the most general interface. But as far as I see, we don't have > any other option here. At least we'd have the theoretical option to construct a > horizontal month view :-) Yes, I didn't enjoy putting the rotated attribute in calIDecoratedView. But as you said, there is no way around this. > Besides what I mentioned above, it bothers me that the "rotate view" option > gets disabled but the checkbox does not. Wouldn't it be much better to not > disable the menu entry at all? Is it wrong to toggle the option while not being > in the week view? At least, the "workweek days only" and "tasks in view" > options also don't get disabled while not being in the calendar. Christian, any > comments on this? Otherwise, I'd prefer to not disable the option at all. I think it would be wrong to have a menu option that doesn't do anything visible in a view. More specifically, If I as a user would select "rotate view" in month view and see that nothing happens, I might think this is a bug and file something like "month view doesn't rotate". But I'd also like a decision from Christian regarding this. I'll upload a new patch as soon as this decision is made.
Comment 21•17 years ago
|
||
I'll could better live with the combination of a disabled label with an always enabled checkbox instead of an entry which is alway enabled. An always enabled menu item would confuse users if there no direct feedback.
Assignee | ||
Comment 22•17 years ago
|
||
Non-bitroted patch, fixes remaining issues.
Attachment #263745 -
Attachment is obsolete: true
Attachment #272386 -
Flags: review?(michael.buettner)
Attachment #263745 -
Flags: review?(michael.buettner)
Reporter | ||
Comment 23•17 years ago
|
||
Philipp, you add the new menu strings in calendar.dtd, but do not remove the obsolete string in calendar.properties. Therefore please remove the string in line 213 of mozilla/calendar/locales/en-US/chrome/calendar/calendar.properties
Assignee | ||
Comment 24•17 years ago
|
||
Ah, yes. Nice catch Simon! This patch also removes the locale string.
Attachment #272386 -
Attachment is obsolete: true
Attachment #274147 -
Flags: review?(michael.buettner)
Attachment #272386 -
Flags: review?(michael.buettner)
Comment 25•17 years ago
|
||
Philipp, you should use |this.viewElem.orient| instead of |document.getAnonymousElementByAttribute(this, "anonid", "view-element").orient|.
Assignee | ||
Comment 26•17 years ago
|
||
I'll keep that in mind when uploading patch after the review, thanks for the tipp!
Comment 27•17 years ago
|
||
Comment on attachment 274147 [details] [diff] [review] Remove rotate button and put in menu - v3 The patch looks fine to me, r=mickey with Martin's previous comment being addressed.
Attachment #274147 -
Flags: review?(michael.buettner) → review+
Assignee | ||
Comment 28•17 years ago
|
||
Checked in on HEAD and MOZILLA_1_8_BRANCH -> FIXED
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Comment 29•17 years ago
|
||
I checked this with the latest Sunbird build (20070731), the task is fixed in sunbird. But this doesn't work in lightning (build 2007073103) correct. If you choose 'Calendar->View->Rotate view" nothing happens. You have to restart thunderbird to get the rotated view.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 30•17 years ago
|
||
This is actually a regression from bug 386714. Lightning's updateOrientation() relied on the id of that deck. Quick one line fix :)
Attachment #274834 -
Flags: review?(michael.buettner)
Comment 31•17 years ago
|
||
Comment on attachment 274834 [details] [diff] [review] Fix regression >+ var deck = document.getElementById("view-deck"); Better use getViewDeck() here. r=mickey with this change. thanks for taking care.
Attachment #274834 -
Flags: review?(michael.buettner) → review+
Assignee | ||
Comment 32•17 years ago
|
||
Regression fix checked in on HEAD and MOZILLA_1_8_BRANCH -> FIXED
Status: REOPENED → RESOLVED
Closed: 17 years ago → 17 years ago
Resolution: --- → FIXED
Comment 33•17 years ago
|
||
Verified in lightning build (2007080603) -> task is fixed.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•