GNOME Bugzilla – Bug 751106
Don't toggle the star button and use a filled/unfilled icon instead of coloring it
Last modified: 2015-10-27 09:22:17 UTC
The star button toggles when you press it, so that a pressed button indicates that a picture has been starred. This doesn't seem right to me, conceptually speaking. A pressed button doesn't say "starred". Changing the color of the icon would be a more effective way to communicate whether an image has been starred - grey for unstarred, and yellow for starred.
I take it!
I seem to recall that the star icon would turn blue when pressed, but that doesn't happen any more. Not sure why. Instead I see a flickering blue when untoggling the icon. :/ Anyway, we need to fix this, and yellow sounds like a better colour than blue.
Created attachment 308722 [details] [review] Adwaita: Colour the star button when is toggled The new colour is yellow
Review of attachment 308722 [details] [review]: Thanks for the patch, Alessandro. Looks good to me, but we need to get this reviewed by a designer / adwaita hacker. ::: data/Adwaita.css @@ +1,3 @@ @define-color theme_base_color #333333; @define-color theme_bg_color #393f3f; +@define-color theme_selected_bg_color #bdaf3b; From where did you get the colour values? @@ +104,3 @@ } +.photos-favorite.button { This fixes the flickering problem. I guess some Adwaita changes broke this some releases ago. Can you split this hunk into a separate patch so that we can backport it to stable branches?
Sorry, I've been meaning to say - perhaps we could used the filled/unfilled variants of the icon, rather than changing the colour (probably better for accessibility!) The icons are starred-symbolic and non-starred-symbolic.
(In reply to Debarshi Ray from comment #4) > > From where did you get the colour values? From a picture (the last one) in Allan's post: https://blogs.gnome.org/aday/2015/07/01/photos-future-plans/
Created attachment 313135 [details] [review] main-toolbar: Use symbolic icon instead of change the colour
Created attachment 313136 [details] [review] Adwaita: Make the favorite button icon work This is a fix for stable branches
Review of attachment 313136 [details] [review]: Thanks Alessandro, it looks good. I pushed it to master [*] and gnome-3-18. It should probably be in gnome-3-16 too, but I can't check right now. [*] Although we are not going to really use it in master, I still prefer to have it there because it makes it easier to understand what was going on.
Review of attachment 313135 [details] [review]: Perfect. I pushed it after rebasing it on top of current master.
Created attachment 313844 [details] [review] Use a filled/unfilled icon instead of colour to denote favorites
Am I the only one who thinks that the non-starred-symbolic looks a bit odd? I agree that it is the right thing to use here, but it looks a bit odd to me. At this size, the white border of the star looks dotted or something. Allan, Jimmac: I guess you also want a normal button, not a toggle, right?
Created attachment 313848 [details] Screenshot of non-starred-symbolic
I think the non starred icon doesn't work without previous knowledge of the starred one. I mean I see a star in a button, it doesn't look "turned off".
Created attachment 313857 [details] [review] main-toolbar: Use a simple button instead of a toggle to denote favorites
Review of attachment 313857 [details] [review]: Thanks, Alessandro. ::: src/photos-main-toolbar.c @@ -463,1 @@ g_signal_handlers_unblock_by_func (priv->favorite_button, photos_main_toolbar_favorite_button_clicked, self); The blocking/unblocking was only needed due to the set_active call. They are useless now. Please remove them.
We need to make the corresponding changes to the selection toolbar too.
Created attachment 313888 [details] [review] Make favorite button consistent in selection toolbar
Created attachment 313889 [details] [review] main-toolbar: Use a simple button instead of a toggle to denote favorites
Created attachment 313890 [details] [review] main-toolbar: Use a simple button instead of a toggle to denote favorites
Review of attachment 313888 [details] [review]: Looks good.
Review of attachment 313890 [details] [review]: Thanks, Alessandro. Looks good now.