GNOME Bugzilla – Bug 709395
We should hide the Users-list button when on selection-mode
Last modified: 2013-10-14 17:33:16 UTC
I think we should hide this button on Selection-Mode. It doesn't make sense to show the current channel's users list while managing/selecting the Rooms' list.
Created attachment 256429 [details] [review] it does the job. :)
(In reply to comment #0) > It doesn't make sense to show the current channel's users list > while managing/selecting the Rooms' list. Yeah, makes sense to me - at least if we keep selection mode, which is not quite clear yet.
Review of attachment 256429 [details] [review]: Only a minor style comment about the code, but I don't think the behavior is quite right - if the user-list is visible when entering selection mode, it seems odd to me to hide the control button but not the sidebar itself. ::: src/mainWindow.js @@ +212,3 @@ this._titlebarLeft.get_style_context().add_class('selection-mode'); this._titlebarRight.get_style_context().add_class('selection-mode'); + this._showUserListButton.hide(); You can use this._showUserListButton.visible = !enabled; above.
(In reply to comment #2) > (In reply to comment #0) > > It doesn't make sense to show the current channel's users list > > while managing/selecting the Rooms' list. > > Yeah, makes sense to me - at least if we keep selection mode, which is not > quite clear yet. Hmm... I didn't considered that. (In reply to comment #3) > Review of attachment 256429 [details] [review]: > > Only a minor style comment about the code, but I don't think the behavior is > quite right - if the user-list is visible when entering selection mode, it > seems odd to me to hide the control button but not the sidebar itself. oh, I missed that! Srry. > > ::: src/mainWindow.js > @@ +212,3 @@ > > this._titlebarLeft.get_style_context().add_class('selection-mode'); > > this._titlebarRight.get_style_context().add_class('selection-mode'); > + this._showUserListButton.hide(); > > You can use > > this._showUserListButton.visible = !enabled; > > above. Clever! ;)
Created attachment 256433 [details] [review] Now it hides the user list too.
Review of attachment 256433 [details] [review]: ::: src/mainWindow.js @@ +216,3 @@ this._titlebarRight.get_style_context().remove_class('selection-mode'); } + Trailing whitespace @@ +218,3 @@ + + this._showUserListButton.visible = !enabled; + this._userListRevealer.visible = !enabled; Sorry, I still don't like this. Just hiding the titlebar button without transition is ok (the whole titlebar changes color without transition, so the abrupt button change is the least of worries there), but I don't think that applies to the sidebar which is a much larger UI element - in my opinion, sliding it out properly is much nicer. With a small change I just pushed[0], this should be as easy as looking up the 'user-list' action and doing action.enabled = !enabled; (also fwiw, I'd slightly prefer this code above the if{}else{} block where there's already similar code, but that's obviously nitpicking) [0] https://git.gnome.org/browse/polari/commit?id=32f038df815d74f0fe
Created attachment 257204 [details] [review] This time looking up at the user-list action to hide the user-list on selection-mode Thx Florian! :)
Review of attachment 257204 [details] [review]: Code looks good, thanks! The commit message should at least contain a link to this bug (see for instance https://git.gnome.org/browse/polari/commit/?id=cb69f7d3a62ea83e15 for the preferred commit message format) - you might want to consider using git-bz[0] to attach patches, which will take care of appending the URL for you. [0] http://blog.fishsoup.net/2008/11/16/git-bz-bugzilla-subcommand-for-git/
Created attachment 257252 [details] [review] mainWindow: Hide user-list while on selection-mode
Review of attachment 257252 [details] [review]: Looks good - do you have git access or should I push the patch for you?
No, I don't.
Attachment 257252 [details] pushed as 65513b2 - mainWindow: Hide user-list while on selection-mode