could you make the weather conditions and current day events like nameday/birthday appear under the calendar too please?

(In reply to Allan Day from comment #0)
>  * Changing the notification icons to 16x16 symbolics

What about the screen shield? Notifications there currently use a bigger icon than the ones in the calendar (48px vs. 32px). Should their look up changed to match the "regular" style from the session, or should they keep bigger icons.

I assume we'll want to at least make those icons symbolic too ...


>  * Removing the media, notification, and event headings

The event heading is currently also used to display the selected date when it's different from today. Do we still want to show that heading, or should it be removed as well?

(Somewhat related: bug 778512)

Let me chime in while Allan is afk. I would keep the same style for the notification on the screen shield. I'll provide a mockup.

For the case of dropping 'Events' section heading, I think it makes sense to keep the heading for when you select a date on the calendar widget as it's emphasizing that events below belong to a particular date.

Created attachment 346861 [details] [review]
messageList: Make canClear public and notify on changes

The latest mockups contain a button to clear all sections at once.
As some elements cannot be cleared, we need to provide that information
to avoid offering an action that has no effect.

Created attachment 346862 [details] [review]
calendar: Add "Clear All" button to message list

We will eventually remove section titles from the message list to
reduce visual noise and give the actual information provided by
the messages more space. So in order to not lose the ability to
mass-dismiss messages, the latest mockups spot a "Clear All" button
at the bottom - implement that.

Created attachment 346863 [details] [review]
messageList: Remove section clear button

With the new "Clear All" button in the message list as a replacement,
we can remove the individual clear buttons from the sections.

Created attachment 346864 [details] [review]
calendar: Only show section title for other days

The section titles usually don't provide a lot of value - messages
themselves are usually pretty unambiguous about their type, and
having a hidden shortcut to some settings panel or application isn't
essential either - except when showing the selected date when browsing
other days, as it adds context to the listed events. Based on that,
remove the section title as a general MessageListSection feature and
move it into the EventsSection, where we only show it when it is useful.

Created attachment 346865 [details] [review]
messageList: Keep secondary actor when showing close button

Currently the secondary actor (if set) and the close button are
exclusive, that is the latter replaces the former on hover. As
the swapping feels rather busy and there's no real reason both
cannot be shown at the same time, keep the secondary actor always
visible.
A welcome side effect is that it no longer needs to be placed at
the end, so we can move the notification timestamp right next to
the corresponding title.

Created attachment 346866 [details] [review]
calendar: Use relative times for notification timestamps

For notifications in the message list, it is usually less relevant
when exactly it occurred, but how long ago. So rather than showing
the exact time and expecting the user to figuring out the timespan
themselves, change the format to something human readable.

Created attachment 346867 [details] [review]
Refine notification style

Update the notification style according to the latest mockups:
 - make notification icons smaller and prefer symbolic variants
 - remove background box when not hovered/focused
 - increase spacing between elements
 - use normal text sizes

Created attachment 346868 [details] [review]
calendar: Add calendar icon to events

Without the boxy background, event messages look a bit plain and
unaligned with other messages. Adding an icon addresses this,
however as repeating the same icon over and over again in case of
many events would be rather noisy, only show it for the top event
as in the mockups.

Review of attachment 346861 [details] [review]:

fine

Review of attachment 346862 [details] [review]:

lgtm

Review of attachment 346863 [details] [review]:

sure

Review of attachment 346864 [details] [review]:

::: js/ui/calendar.js
@@ +816,3 @@
+        this._title = new St.Button({ style_class: 'events-section-title',
+                                      label: '', x_align: St.Align.START,
+                                      can_focus: true });

one property per line please, or things might easily get overlooked.

did you avoid x_expand on purpose?

Review of attachment 346865 [details] [review]:

looks good

(In reply to Rui Matos from comment #15)
> one property per line please, or things might easily get overlooked.

OK.


> did you avoid x_expand on purpose?

Kind of - as mentioned in the other bug, we get this via the :x-fill child property already. Of course it wouldn't hurt keeping it ...

Review of attachment 346866 [details] [review]:

code looks good, feel free to disregard my comments below

::: js/misc/util.js
@@ +173,3 @@
+    let daysAgo = timespan / GLib.TIME_SPAN_DAY;
+    let weeksAgo = daysAgo / 7;
+    let yearsAgo = weeksAgo / 52;

personally I'd prefer to see weeks up until 8 weeks and months till 1 year

@@ +178,3 @@
+        return _("Just now");
+    if (hoursAgo < 1)
+        return _("A while ago")

this seems really vague to me. did you consider "just now" for < 5 mins and "%d minutes ago" for 10 min intervals afterwards i.e. would only show 10 for [5,15], 20 for [15,25], etc. ?

Review of attachment 346867 [details] [review]:

ok

Review of attachment 346868 [details] [review]:

looks fine

unrelated design comment: would be nice to do the same icon hiding for clustered notification messages from the same source. I guess a problem there is that the icon used might be different...

Created attachment 346932 [details] [review]
calendar: Use relative times for notification timestamps

(In reply to Rui Matos from comment #18)
> +    let weeksAgo = daysAgo / 7;
> +    let yearsAgo = weeksAgo / 52;
> 
> personally I'd prefer to see weeks up until 8 weeks and months till 1 year

I doubt that you (or anyone really) would personally see notifications that are older than 8 weeks - I was actually considering using something like "A long time ago" for that :-)

But yeah, doesn't hurt to do it properly even in a range we don't expect users to run into.


> @@ +178,3 @@
> +        return _("A while ago")
> 
> this seems really vague to me. did you consider "just now" for < 5 mins

Allan agreed to the 5 minutes and that "A while ago" is too vague, but suggested to just use the actual minutes.

Review of attachment 346932 [details] [review]:

ok, looks fine

Attachment 346861 [details] pushed as 239b67e - messageList: Make canClear public and notify on changes
Attachment 346862 [details] pushed as d3bb790 - calendar: Add "Clear All" button to message list
Attachment 346863 [details] pushed as 8a6157c - messageList: Remove section clear button
Attachment 346864 [details] pushed as fec511c - calendar: Only show section title for other days
Attachment 346865 [details] pushed as c4f2bb5 - messageList: Keep secondary actor when showing close button
Attachment 346867 [details] pushed as d3c050b - Refine notification style
Attachment 346868 [details] pushed as 846e3f8 - calendar: Add calendar icon to events
Attachment 346932 [details] pushed as f3d1c78 - calendar: Use relative times for notification timestamps

> notify-send "short"

I can be wrong, but with short messages close button on message is moved to the left.

all fine, just ignore my message.