GNOME Bugzilla – Bug 756014
Revamp empty states
Last modified: 2021-07-05 11:30:33 UTC
We should follow the empty states design: https://wiki.gnome.org/Design/OS/EmptyStates
Created attachment 312603 [details] [review] Remove unecessary GtkGrid
Created attachment 312604 [details] [review] view: Use different empty states depending on which mode is used
Created attachment 312605 [details] video of the current changes
Thanks for picking this up, Alessandro. The mock-ups have disabled the stack switcher and the search button, which is not the case in your screencast. Now, even if we do that, we need to figure out how to handle having some documents, but no collections or starred items.
Looks great to me! Debarshi's right that the stack switcher and search button should be disabled when there aren't any documents. One tiny niggle, the subtext is a bit far away from the heading above.
Created attachment 315783 [details] [review] Remove unecessary GtkGrid
Created attachment 315784 [details] [review] view: Use different empty states depending on which mode is used
Created attachment 315786 [details] [review] mainToolbar: Disable headerbar controls when there are no documents
Review of attachment 315783 [details] [review]: Good catch. Maybe add the "view: " prefix to the commit message?
Review of attachment 315784 [details] [review]: ::: src/view.js @@ +284,3 @@ + + if (this._mode == WindowMode.WindowMode.COLLECTIONS) { + this.add(new Gtk.Label({ label: _("You can create collections from the Recent view"), margin_top: 18 })); For what it's worth, in gnome-photos, the secondary text for the "no collections" case is: "Name your first album"
The primary text in nautilus is "Folder is Empty", while the mockups [1] use "No documents found" - different capitalization formats. :) [1] https://wiki.gnome.org/Design/OS/EmptyStates
Review of attachment 315786 [details] [review]: ::: src/mainToolbar.js @@ +150,3 @@ + this._selectionButton.set_sensitive(true); + this._viewMenuButton.set_sensitive(true); + } Can't we use gtk_container_foreach or gtk_container_get_children to iterate over the children instead of tracking them manually?
Created attachment 316835 [details] [review] mainToolbar: Disable headerbar controls when there are no documents
Review of attachment 316835 [details] [review]: Thanks Alessandro. This looks much better. ::: src/mainToolbar.js @@ +142,3 @@ + function(controller, count) { + this.toolbar.foreach(function(child) { child.set_sensitive(count != 0); }); + })); It would be better to move this chunk to _resetToolbar. Otherwise, if you toggle selection mode, then the signal handler will be disconnected without any reconnection, and break the logic. Nitpick: break the function(..) into a separate lines like we do in view.js.
By the way, now that we are disabling the UI controls in the header bar, we should take care of the case where the searchbar was on, and all the documents got removed in the background. We should probably switch it off in that case. This isn't a very edge scenario if you consider the case where all the user documents are coming from a online provider and the user turned off the online account while the search bar was on.
(In reply to Debarshi Ray from comment #15) > By the way, now that we are disabling the UI controls in the header bar, we > should take care of the case where the searchbar was on, and all the > documents got removed in the background. We should probably switch it off in > that case. > > This isn't a very edge scenario if you consider the case where all the user > documents are coming from a online provider and the user turned off the > online account while the search bar was on. We can handle that in a separate patch. :)
Review of attachment 315784 [details] [review]: ::: src/view.js @@ +284,3 @@ + + if (this._mode == WindowMode.WindowMode.COLLECTIONS) { + this.add(new Gtk.Label({ label: _("You can create collections from the Recent view"), margin_top: 18 })); Oh, and there is no "Recent" view anymore. It is just "Documents" now.
Created attachment 316933 [details] Screenshot - no content whatsoever, but online accounts present
Created attachment 316934 [details] Screenshot - no collections, but online accounts present
Created attachment 316941 [details] Screenshot - no content whatsoever, no online accounts present
This is looking really great! (In reply to Debarshi Ray from comment #18) > Created attachment 316933 [details] > Screenshot - no content whatsoever, but online accounts present It looks a bit empty/unhelpful without the explanation text in this case. Why not include it? (In reply to Alessandro Bono from comment #20) > Created attachment 316941 [details] > Screenshot - no content whatsoever, no online accounts present Shouldn't the header bar controls be insensitive in this case?
(In reply to Allan Day from comment #21) > This is looking really great! > > (In reply to Debarshi Ray from comment #18) > > Created attachment 316933 [details] > > Screenshot - no content whatsoever, but online accounts present > > It looks a bit empty/unhelpful without the explanation text in this case. > Why not include it? Because in the current version (3.18) Documents shows the secondary text only if the user doesn't have a Online Account and I've kept the same behavior. I'll add the text in any case. > > (In reply to Alessandro Bono from comment #20) > > Created attachment 316941 [details] > > Screenshot - no content whatsoever, no online accounts present > > Shouldn't the header bar controls be insensitive in this case? Yes they should and the patch does it. I used an old screenshot to show mainly the secondary text.
Created attachment 319059 [details] [review] view: Use different empty states depending on which mode is used
Created attachment 319060 [details] [review] mainToolbar: Disable headerbar controls when there are no documents
Created attachment 319061 [details] [review] search: Remove unused functions
Created attachment 319062 [details] [review] mainToolbar: Hide searchbar when there are no documents
Created attachment 319063 [details] [review] mainToolbar: Switch to Documents view when there are no documents
The last patch doesn't actually switch to the Documents view because offsetDocumentsController doesn't emit the signal item-count-changed when we are in the Collections view.
Review of attachment 319060 [details] [review]: Looks good to me, thanks Alessandro.
Review of attachment 319062 [details] [review]: Nice patch. One minor issue, though. ::: src/mainToolbar.js @@ +338,3 @@ + let searchAction = Application.application.lookup_action('search'); + if (count == 0 && searchAction.state.get_boolean()) + searchAction.state = GLib.Variant.new('b', false); *Users* of a GAction are not supposed to directly change the 'state' property. See: https://developer.gnome.org/gio/stable/GSimpleAction.html Instead, use application.change_action_state or searchAction.change_state. We usually use the first one in the rest of the code.
Review of attachment 319062 [details] [review]: ::: src/mainToolbar.js @@ +338,3 @@ + let searchAction = Application.application.lookup_action('search'); + if (count == 0 && searchAction.state.get_boolean()) + searchAction.state = GLib.Variant.new('b', false); Actually, in this case, we should disable the action too. Otherwise one can still do ctrl+f to reveal the searchbar. You might need to lookup the action after all.
Review of attachment 319063 [details] [review]: Other than what you already mentioned in comment 28 ... Won't this also change the mode when there are documents, but nothing to show for the current search results? It doesn't seem to be doing that right now, and I am not sure why. If you only want to move out of the current collection, then activatePreviousCollection is the right thing to use.
Review of attachment 319061 [details] [review]: I am undecided about this one. Even if we are not using those methods, it seems reasonable to expect that the OffsetController class would have them. They are closely related to the semantics of the class, and we might need them in future. I don't know. *shrug*
Review of attachment 319059 [details] [review]: This looks really good. ::: src/view.js @@ +269,3 @@ + } else { + text = Application.application.isBooks ? _("No books found") : _("No documents found"); + } Nitpick. No braces for one-line if-else branches. @@ +287,3 @@ + else + label = _("You can create collections from the Documents view"); + this.add(new Gtk.Label({ label: label, margin_top: 9 })); Maybe, instead of setting the margin on each widget, just use row_spacing on the top-level grid? And set the margin when you want some extra spacing between the first two rows? Also, we usually use multiples of 6 for spacing. @@ +296,3 @@ let detailsStr = // Translators: %s here is "Settings", which is in a separate string due to // markup, and should be translated only in the context of this sentence This comment also needs an update to avoid confusion for translators.
Created attachment 319696 [details] [review] view: Implement the new empty states design Pushed after making the above adjustments.
Created attachment 320633 [details] [review] mainToolbar: Hide searchbar when there are no documents
(In reply to Debarshi Ray from comment #32) > Review of attachment 319063 [details] [review] [review]: > > Other than what you already mentioned in comment 28 ... > > Won't this also change the mode when there are documents, but nothing to > show for the current search results? It doesn't seem to be doing that right > now, and I am not sure why. Sorry, I was wrong. We are using Application.offsetDocumentsController, which tracks the documents in the "Documents" view. This makes sense because if there are no documents, then it doesn't matter whether we have collections or not. > If you only want to move out of the current collection, then > activatePreviousCollection is the right thing to use. This part is still valid, I think. ie., if we are inside a collection, then the right way to get out of it is to use activatePreviousCollection
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new ticket at https://gitlab.gnome.org/GNOME/gnome-documents/-/issues/ Thank you for your understanding and your help.