Adapt the UI when users are not using Calendar
Categories
(Calendar :: General, enhancement)
Tracking
(Not tracked)
People
(Reporter: pmorris, Assigned: pmorris)
References
Details
Attachments
(2 files, 5 obsolete files)
5.39 KB,
patch
|
pmorris
:
review+
|
Details | Diff | Splinter Review |
30.90 KB,
patch
|
pmorris
:
review+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #1592987 +++
According to the plan worked out in bug 1592987, the calendar UI will adapt when users are not using calendar functionality. Namely, the UI will adapt when users have all calendars disabled (i.e. calendar properties -> uncheck "switch this calendar on"). This bug is to implement this functionality. Here's the current plan for how various UI items will adapt:
NO CHANGE
- "Create a new calendar" link on account central page
- calendar and task buttons in the tabs bar (they may be relocated eventually)
HIDDEN
- today pane
- today pane button in the status bar
- "Events and Tasks" menu
SOME ITEMS HIDDEN, SOME ITEMS VISIBLE AND USABLE
- menu items in "File", "Edit", etc.
- menu items in context menus
- keyboard shortcut keys (some disabled, others still work, following the corresponding menu items)
SPECIAL CASES
- IMIP bar ("this message contains an invitation...")
Always show this notification to allow users to quickly add the event. If calendar code is disabled, include an option for "don't offer to add any more calendar events". - Calendar, Tasks, Event, and Task tabs
Basically unchanged, but if calendar is unused, show a message to explain the current status, and provide a smooth path for users to start using calendar features.
Assignee | ||
Comment 1•4 years ago
|
||
Uploading a WIP patch for early feedback on the general approach.
With this patch, when all calendars are disabled (right click on calendar in calendar list, select "Properties", uncheck "switch this calendar on"), the today pane, today pane button, "Events and Tasks" menu (in both menu bar and app menu) are hidden. The today pane's per tab shown/hidden state is restored when re-enabling a calendar.
Next step is to add menu items and keyboard shortcut keys. I may handle the "special cases" (see previous comment) separately from the rest.
Assignee | ||
Updated•4 years ago
|
Comment 2•4 years ago
|
||
Comment on attachment 9128680 [details] [diff] [review] wip-calendar-deactivator-0.patch Review of attachment 9128680 [details] [diff] [review]: ----------------------------------------------------------------- What if the deactivator was in a JSM and handled the enabling/disabling events once for all windows? A bit like the colour tracker in calViewUtils.jsm. You could also do the hiding by CSS β just set an attribute on the root element and have rules that match it β which would make it independent of the logic.
Comment 3•4 years ago
|
||
Comment on attachment 9128680 [details] [diff] [review] wip-calendar-deactivator-0.patch Review of attachment 9128680 [details] [diff] [review]: ----------------------------------------------------------------- I like this approach, just a tiny suggestion on a condition. I know this is not a review, so apologies if my comment was annoying. ::: calendar/base/content/calendar-deactivator.js @@ +54,5 @@ > + this.idsToHide.forEach(id => (document.getElementById(id).hidden = false)); > + } else { > + this.idsToHide.forEach(id => (document.getElementById(id).hidden = true)); > + } > + } This looks a bit verbose. What if we interrupt if the variables match? ``` if (someCalsEnabled == this.mIsActivated) { return; } ``` Also, we should use `for` instead of `forEach` as it's drastically faster. It probably doesn't matter much since we're looping through and array of strings anyway, and not nodes, but you could do something like this: ``` for (id of this.idsToHide) { document.getElementById(id).hidden = someCalsEnabled; }
Assignee | ||
Comment 4•4 years ago
•
|
||
Thanks Geoff and Alex for the helpful and timely feedback.
(In reply to Geoff Lankow (:darktrojan) from comment #2)
What if the deactivator was in a JSM and handled the enabling/disabling
events once for all windows? A bit like the colour tracker in
calViewUtils.jsm.You could also do the hiding by CSS β just set an attribute on the root
element and have rules that match it β which would make it independent of
the logic.
I've done both these now and it's much better, thanks. I initially tried a JSM but retreated since you can't access the window/document directly from a JSM. Following the approach in calViewUtils.jsm makes that a non-issue.
(In reply to Alessandro Castellani (:aleca) from comment #3)
This looks a bit verbose.
What if we interrupt if the variables match?
I generally prefer to avoid that return early pattern (it's simpler if functions have a single return rather than multiple), but we use it a lot, so it probably makes sense here.
Also, we should use
for
instead offorEach
as it's drastically faster.
Yeah, switched to using for
in one place (also because calendars is now a set rather than an array), although using CSS now avoids that loop entirely.
Assignee | ||
Comment 5•4 years ago
|
||
Ready for review. This handles the most obvious things, hiding them when all calendars are disabled:
HIDDEN
- today pane
- today pane button in the status bar
- "Events and Tasks" menu
SOME ITEMS HIDDEN, SOME ITEMS VISIBLE AND USABLE
- menu items in "File", "Edit", etc.
I didn't find any menu items in context menus that needed to be hidden. For keyboard shortcuts I found a couple <command>
s to disable ("calendar_toggle_todaypane_command", "calendar_go_to_today_command"). Luckily the toggle_todaypane keyboard shortcut is already getting disabled without any extra effort required. The go_to_today one is not, but it's tricky because even if you disable it, when you open a calendar tab it gets re-enabled. (Disabling it is pretty low priority. If a user accidentally hits alt+end in a mail tab a calendar tab will open, which isn't so bad but not ideal.)
Anyway, I've left out disabling commands for now, and I'd like to handle that and other items in a follow-up so we can go ahead and get the basics in place.
Try run just started: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=74406d0abc59360a2852b95ae042b7c6994aa091
Comment 6•4 years ago
|
||
Instead of one giant selector with all the IDs, I think it'd be better to have an attribute or class set on every element affected. That way the addition of a new piece of UI doesn't require finding the giant selector in the CSS and modifying it. Not sure what impact there would be on performance but I don't imagine it's terrible.
I didn't find any menu items in context menus that needed to be hidden.
What about #mailContext-calendar-convert-menu? Did you miss it or decide it should remain? I can see an argument either way.
Assignee | ||
Comment 7•4 years ago
|
||
(In reply to Geoff Lankow (:darktrojan) from comment #6)
Instead of one giant selector with all the IDs, I think it'd be better to have an attribute or class set on every element affected. That way the addition of a new piece of UI doesn't require finding the giant selector in the CSS and modifying it. Not sure what impact there would be on performance but I don't imagine it's terrible.
That makes sense to me. Funny enough, it's what I had started to do in the previous version of the patch, with a 'hide-when-calender-deactivated' class on the various elements. I asked Paenglab about it in chat earlier today and he steered me in the current direction. Here's his comment (I had used a "calendar-disabled" class rather than an attribute):
How about using an attribute on the window? Then you could use :root[calendar-disabled] selector {}. Not sure but this could be probably faster than the class. We use this a lot for theme things. And instead of using the hide-when-calendar-deactivated class, hide directly the element with the :root[calendar-disabled] selector {}.
(In reply to Geoff Lankow (:darktrojan) from comment #6)
What about #mailContext-calendar-convert-menu? Did you miss it or decide it should remain? I can see an argument either way.
Oh good catch, I missed it. I think that would be a good candidate for hiding. I just checked and found that it is disabled when all calendars are disabled.
Comment 8•4 years ago
|
||
It was only a proposal. I'm also okay with the class.
Comment 9•4 years ago
|
||
(In reply to Geoff Lankow (:darktrojan) from comment #6)
Instead of one giant selector with all the IDs, I think it'd be better to have an attribute or class set on every element affected. That way the addition of a new piece of UI doesn't require finding the giant selector in the CSS and modifying it. Not sure what impact there would be on performance but I don't imagine it's terrible.
I didn't find any menu items in context menus that needed to be hidden.
What about #mailContext-calendar-convert-menu? Did you miss it or decide it should remain? I can see an argument either way.
Should not trade off end user speed for ease of use to programmers. The ratio is just too far off, especially if the programmers are also users. Must do everything possible to improve and NOT degrade the speed.
Assignee | ||
Comment 10•4 years ago
•
|
||
Removes the use of 'ltn' as a prefix in element ids outside of the calendar directory. This will make it easier to identify calendar UI elements in mail code outside of the calendar directory. Eventually it would be good to remove all uses of 'ltn' but there are quite a few (looks like between 100 and 200 for use in ids).
Assignee | ||
Comment 11•4 years ago
|
||
(In reply to Richard Marti (:Paenglab) from comment #8)
It was only a proposal. I'm also okay with the class.
Okay, thanks! Now using a class (rather than ids) to target the elements that are hidden when calendar is deactivated. Now also hides the mailContext-calendar-convert-menu
menuitem.
(In reply to Worcester12345 from comment #9)
Should not trade off end user speed for ease of use to programmers.
I'd expect that in this case the difference in speed would be negligible.
Updated•4 years ago
|
Comment 12•4 years ago
|
||
Comment on attachment 9129872 [details] [diff] [review] part2-calendar-deactivator-3.patch Review of attachment 9129872 [details] [diff] [review]: ----------------------------------------------------------------- ::: calendar/base/modules/calCalendarDeactivator.jsm @@ +92,5 @@ > + // calICalendarManagerObserver methods > + onCalendarRegistered(calendar) { > + this.calendars.add(calendar); > + > + if (!calendar.getProperty("disabled")) { You could skip this check, if isCalendarActivated is true. Alternatively you could always refresh the state. Using only one of the things you already know here, while not wrong, seems a bit odd. Do you see what I mean? ::: mail/components/customizableui/content/panelUI.inc.xhtml @@ +163,5 @@ > class="subviewbutton subviewbutton-iconic" > label="&newNewMsgCmd.label;" > key="key_newMessage2" > command="cmd_newMessage"/> > + <toolbarbutton id="appmenu_calendar-new-event-menu-item hide-when-calendar-deactivated" That's not right.
Assignee | ||
Comment 13•4 years ago
|
||
I did a try run, which looks fine. The failures are due to the unrelated "test_sort_orders.js" issue.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=eada8332bbf623c62f0ab125b129f40d2b9bd6d3&selectedJob=291272101
This patch fixes the linting error flagged in the try run.
Assignee | ||
Comment 14•4 years ago
|
||
Thanks for the review. Makes sense, and good catch. This fixes those two things.
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Updated•4 years ago
|
Comment 15•4 years ago
|
||
Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/77c269c63ddc
Avoid using 'ltn' prefix outside calendar directory. r=darktrojan
https://hg.mozilla.org/comm-central/rev/46b1f5963bc7
Hide part of calendar UI when all calendars are disabled. r=darktrojan
Updated•4 years ago
|
Description
•