A couple of questions for Allan:

1 - (not specific to clocks) the new pattern makes it less evident, but still has the issue of the action bar potentially covering the some of the selected items... should we scroll the view? should we just live with it? (I am fine with the latter)

2 - Does the bar at the bottom use a standard css class?

3 - For clocks where we just have a single "delete" action, should it go on the left or on the right?


If someone wants to give this task a try it should be fairly easy, the bottom bar can be implement with GtkRevealer.

Created attachment 245246 [details] [review]
patch proposal for the ui rework

and here are additional sreenshots for the first draft of the UI selection pattern redesign

Created attachment 245247 [details]
screenshot 1: delete button is not sensible

Created attachment 245248 [details]
second screenshot

Review of attachment 245246 [details] [review]:

Could you attach screenshots with the default adwaita theme?


Patch looks mostly good, some comments below.

::: src/widgets.vala
@@ +405,3 @@
         scrolled_window.add (icon_view);
 
+        box = new Gtk.Box (Gtk.Orientation.VERTICAL, 0);

I'd prefer to use GtkGrid instead of Box

@@ -465,3 +471,3 @@
     private Gtk.Toolbar create_selection_toolbar () {
         var toolbar = new Gtk.Toolbar ();
-        toolbar.show_arrow = false;
+

I think we should set the "primary toolbar" css class, unless there is a specific class for this bar. Allan?

@@ -467,5 +473,5 @@
-        toolbar.show_arrow = false;
-        toolbar.margin_bottom = 40;
-        toolbar.icon_size = Gtk.IconSize.LARGE_TOOLBAR;
... 2 more ...
+        delete_button.label = _("Delete");
+        delete_button = new Gd.HeaderSimpleButton ();
+        delete_button.get_style_context ().add_class ("suggested-action");
... 2 more ...

I think the Delete button should be at the right since it is the only action in this case.

@@ -515,4 +515,2 @@
     }
 
-    private void fade_in (Gtk.Widget w) {
-        uint timeout_id = w.get_data<uint> ("cloks-fade-out-timeout-id");

The patch should also remove these styles from the css since they are not used anymore

sure, this was actually the default adwaita theme)) but in dark))

Created attachment 245645 [details]
screenshot 1: delete button is not sensible

Created attachment 245646 [details]
second screenshot

I really thought I'd replied to this! Sorry to both.

(In reply to comment #1)
> A couple of questions for Allan:
> 
> 1 - (not specific to clocks) the new pattern makes it less evident, but still
> has the issue of the action bar potentially covering the some of the selected
> items... should we scroll the view? should we just live with it? (I am fine
> with the latter)

Yes, scroll the view.

> 2 - Does the bar at the bottom use a standard css class?

Yes, just treat it as a standard primary toolbar.

> 3 - For clocks where we just have a single "delete" action, should it go on the
> left or on the right?

I'd go for right.

(In reply to comment #9)
> Created an attachment (id=245646) [details]
> second screenshot

Hey Evgeny! This looks great! Thanks for working on it.

There are a few tweaks that I would recommend:

 * The bottom toolbar is missing a stroke on the top edge. I know that this is something that other applications, like Contacts and Music have had to solve, so I would speak to their developers. You could also ask lapo or cosimoc, since they are our theme experts.

 * As noted above, I would put the delete button on the right. This is mostly because it is a destructive action, so putting it slightly more out of focus feels more appropriate.

 * I wouldn't make the delete button blue, and would instead use the standard style. We use blue for the default action, which shouldn't be coupled to a destructive action like delete.

(In reply to comment #6)
> Review of attachment 245246 [details] [review]:
> 
> Could you attach screenshots with the default adwaita theme?
> 
> 
> Patch looks mostly good, some comments below.
> 
> ::: src/widgets.vala
> @@ +405,3 @@
>          scrolled_window.add (icon_view);
> 
> +        box = new Gtk.Box (Gtk.Orientation.VERTICAL, 0);
> 
> I'd prefer to use GtkGrid instead of Box
okey
> 
> @@ -465,3 +471,3 @@
>      private Gtk.Toolbar create_selection_toolbar () {
>          var toolbar = new Gtk.Toolbar ();
> -        toolbar.show_arrow = false;
> +
> 
> I think we should set the "primary toolbar" css class, unless there is a
> specific class for this bar. Allan?
I will attach a screenshot of a toolbar with a "primary-toolbar" css set. looks odd to me and the one you have suggested with:

.clocks-selection-bar {
    border-top: 1px;
    border-style: solid;
    border-color: @borders;
}

> 
> @@ -467,5 +473,5 @@
> -        toolbar.show_arrow = false;
> -        toolbar.margin_bottom = 40;
> -        toolbar.icon_size = Gtk.IconSize.LARGE_TOOLBAR;
> ... 2 more ...
> +        delete_button.label = _("Delete");
> +        delete_button = new Gd.HeaderSimpleButton ();
> +        delete_button.get_style_context ().add_class ("suggested-action");
> ... 2 more ...
> 
> I think the Delete button should be at the right since it is the only action in
> this case.
it's from now on on the right hand side
> 
> @@ -515,4 +515,2 @@
>      }
> 
> -    private void fade_in (Gtk.Widget w) {
> -        uint timeout_id = w.get_data<uint> ("cloks-fade-out-timeout-id");
> 
> The patch should also remove these styles from the css since they are not used
> anymore
thanks, indeed this should be removed. I overlooked it.

Created attachment 245993 [details]
screenshot 1: with primariy-toolbar css. delete button is not sensible

Created attachment 245994 [details]
screenshot 2: with primariy-toolbar css. delete button is sensitive

with custom css style:

.clocks-selection-bar {
    border-top: 1px;
    border-style: solid;
    border-color: @borders;
}

Created attachment 245995 [details]
screenshot 1: with custom css. delete button is not sensitive

Created attachment 245996 [details]
screenshot 2: with custom css. delete button is sensitive

(In reply to comment #11)
> (In reply to comment #9)
> > Created an attachment (id=245646) [details] [details]
> > second screenshot
> 
> Hey Evgeny! This looks great! Thanks for working on it.
Hey Allan, it's a pleasure for me. Moreover, I have been accepted and will work within the frame of gsoc on gnome-clocks picture support and geolocation integration in view.
> 
> There are a few tweaks that I would recommend:
> 
>  * The bottom toolbar is missing a stroke on the top edge. I know that this is
> something that other applications, like Contacts and Music have had to solve,
> so I would speak to their developers. You could also ask lapo or cosimoc, since
> they are our theme experts.
Thank you, I have talked with them and Paolo has helped me with that as well.
> 
>  * As noted above, I would put the delete button on the right. This is mostly
> because it is a destructive action, so putting it slightly more out of focus
> feels more appropriate.
done.
> 
>  * I wouldn't make the delete button blue, and would instead use the standard
> style. We use blue for the default action, which shouldn't be coupled to a
> destructive action like delete.
incorporated. So, please take a fresh look)

Created attachment 246000 [details] [review]
patch proposal for the ui rework

Created attachment 246009 [details] [review]
patch proposal for the ui rework

Created attachment 246010 [details] [review]
patch proposal for the ui rework

The updated screenshots look perfect! It would be preferable not to use custom css styling here - if you've gone down that route, it would be worth talking to Paolo and the other application developers about getting this fixed in Adwaita.

Thanks again for working on this. :)

the custom css is just for the upper border, I think it is ok for now. Once the pattern is standardized we can move it to adwaita and drop our copy


The patch looks good and I will push it asap

I have tweaked the css a little bit and pushed. Thanks!

Thanks Evgeny! I'm looking forward to trying this.