GNOME Bugzilla – Bug 779368
Account dialogs - put expired credentials UI in infobars
Last modified: 2017-06-05 13:05:40 UTC
I have to hold my hands up here - the expired credentials part of the account dialogs wasn't great in terms of design. The goal was to avoid having to move the rest of the dialog elements, but the result was that the credentials part looks out of place and makes the rest of the layout a bit awkward. The current fine for 3.24 but in future it would be good to move the expired credentials part of the dialogs into infobars, as shown in these updated mockups: https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/online-accounts/aday-alt/online-accounts-account-dialogs.png
Created attachment 350595 [details] [review] online-accounts: Tweak account editor dialog In order to move the expired credentials button to an info bar above the other widgets, we need to tweak Control Center's dialog to have borders that don't affect the GOA widgets.
Created attachment 350596 [details] [review] provider: Make expired credentials an info bar (This patch is to be applied on gnome-online-accounts) Per the mockups at [1], when the account editor dialog displays the expired credentials widgets, they should be packed inside a GtkInfoBar. This patch adds an info bar above the expired credential widgets, and adjusts the code to tweak the dialog. [1] https://raw.githubusercontent.com/gnome-design-team/gnome-mockups/master/system-settings/online-accounts/aday-alt/online-accounts-account-dialogs.png
Review of attachment 350595 [details] [review]: Thanks for the patches, Georges! What's with the super narrow commit messages? :) Would be nice to explain why the margins had to be moved around. Having the UI spread across two modules can make it tricky to follow the logic. ::: panels/online-accounts/online-accounts.ui @@ +260,3 @@ <property name="can_focus">True</property> + <property name="margin-start">36</property> + <property name="margin-end">36</property> Why 36, not 24? (see below)
Review of attachment 350596 [details] [review]: ::: src/goabackend/goaprovider.c @@ +595,3 @@ gtk_grid_set_column_spacing (GTK_GRID (grid), 12); gtk_grid_set_row_spacing (GTK_GRID (grid), 6); + gtk_widget_set_margin_bottom (grid, 24); Are you sure that you didn't intend to set the top margin? Earlier, accounts_vbox's parent GtkBox had a margin of 24. To accommodate the InfoBar, it only has a bottom-margin now. So, I assume you wanted to set the other margins here? Note that accounts_vbox and remove_account_button already has 24 pixels between them due to their parent GtkBox's spacing. So, if we want more spacing between them we can just increase the spacing, no? Just to avoid spreading the values too much and keep the arithmetic simple. Also, having more of this stuff outside the vfunc avoids the need to check and keep the other implementations updated. @@ +597,3 @@ + gtk_widget_set_margin_bottom (grid, 24); + gtk_widget_set_margin_start (grid, 24); + gtk_widget_set_margin_end (grid, 24); Shouldn't the horizontal margins be 42 (= 24 + 18)? Currently the Grid's margins are narrower than that of remove_account_button, which breaks the layout - the Switches are too close to the edge. The mockups seem to suggest even wider margins, but we can do that later. @@ +598,3 @@ + gtk_widget_set_margin_start (grid, 24); + gtk_widget_set_margin_end (grid, 24); + gtk_container_add (GTK_CONTAINER (vbox), grid); These kind of cosmetic changes are best done in a separate patch. ::: src/goabackend/goatelepathyprovider.c @@ +947,1 @@ grid = gtk_grid_new (); We need to set the margins on this Grid too. ::: src/goabackend/goautils.c @@ +120,3 @@ + attention_needed = goa_account_get_attention_needed (account); + + gtk_widget_set_margin_top (GTK_WIDGET (vbox), attention_needed ? 0 : 32); Umm... I don't understand this. When attention_needed is FALSE accounts_vbox's (and the Grid's) top margin is 32. When TRUE, there is only a 18 pixel gap between the Grid and the InfoBar. Shouldn't they be the same?
Created attachment 352873 [details] [review] online-accounts: Move the margins and spacing lower down the hierarchy I took the liberty to make the above adjustments.
Review of attachment 350596 [details] [review]: Forgot to mention that the show_accounts vfunc for the IMAP/SMTP provider needs to be updated too.
Created attachment 352874 [details] [review] provider: Use an info bar for accounts needing attention I split out the gtk_box_pack_start -> gtk_container_add changes, and updated the Telepathy and IMAP/SMTP providers.
Created attachment 352875 [details] IMAP/SMTP - needs attention
Created attachment 352876 [details] Google - needs attention
Created attachment 352877 [details] Nextcloud
Created attachment 352878 [details] [review] imap-smtp: providers, telepathy: Use GtkContainer API instead of GtkBox
Created attachment 352879 [details] [review] online-accounts: Move the margins and spacing lower down the hierarchy Let's also bump the gnome-online-accounts requirement since the UI code is so tightly coupled.
Comment on attachment 352874 [details] [review] provider: Use an info bar for accounts needing attention Pushed to master
Comment on attachment 352878 [details] [review] imap-smtp: providers, telepathy: Use GtkContainer API instead of GtkBox Pushed to master.
Comparing the attached screenshots and the mockups, it seems to me that we need a wider horizontal margin for the account details widgets, and more spacing above the "remove account" button. Not really related to this bug, but ... *shrug*
Created attachment 352881 [details] [review] imap-smtp: providers, telepathy: Increase horizontal margin
Created attachment 352882 [details] [review] online-accounts: Increase the spacing above the "Remove Account" button
Allan is on vacation. So I am pushing the patches to increase the margins and spacing for the time being to get this bug out of my queue. We can adjust or revert later if required. Thanks for all the work here, Georges!
This looks good! My only comment would be that the warning icons aren't needed, and that the second line of text in the infobar looks a bit faint. It might be better to use bold for the heading and then standard white text for the description.
Created attachment 353088 [details] [review] utils: Remove the warning icon from the needs-attention GtkInfoBar
Created attachment 353089 [details] [review] utils: Embolden the heading instead of dimming the description
Created attachment 353090 [details] [review] utils: Add some margin around the contents of the GtkInfoBar Don't we need some horizontal margin inside the GtkInfoBar? How about this?
Created attachment 353091 [details] Google - needs attention
Created attachment 353092 [details] IMAP/SMTP - needs attention
Looks like we forgot to update the header string: "Credentials have expired." vs "Connection has expired"
Looks good. Headings don't need full stops at the end, and they could do with more padding.
Created attachment 353180 [details] [review] utils: Remove the full stop from the GtkInfoBar's header string