-
Notifications
You must be signed in to change notification settings - Fork 703
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change close button behaviour #184
Conversation
Hey @xPaw, trying this out, it appears that closing the non-active channel by accident is still possible, as its opacity is 0 but its bounding box is still there: |
1bc898d
to
a279f43
Compare
@astorije fixed. |
background-color: rgba(0, 0, 0, .1); | ||
opacity: .7 !important; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One less !important
, yay!
Very cool @xPaw, 👍! |
Hmm, I'm not sure I like the removal of the dedicated Leave button in the actual chat window. It's not a huge deal, but I used that button when being done with PMs or leaving channels. Reason being, it's always at the same place and it's always obvious what window it will close. Do we have a specific reason why we want to remove that one? |
That's the only way I close PMs on mobile, so if it is removed I would vote for hiding it on desktop only. |
@dgw Ouch, that is indeed a very fair point there. I'm definitely against the removal of the button then. Actually I don't like having the close buttons in the sidebar at all on mobile, those are really, really easy to fatfinger and mistap. Way more than the hidden X on desktop. |
Yes, please read #61 (comment) have some details about it.
Not anymore: with the next release will come the context menu on right/long click, and this PR also displays the × all the time on the active channel. So essentially, this is just moving an action, and will only require a bit of getting used to.
There is no more chance of fatfingering this than any other button icon on this app or any other. And actually, very little chance of fatfingering this as the button only appears in the active channel. I know these minor details always bring up the most controversy, but after the long debate on #61 and exploring different ideas, I really hoped we'd have reached consensus here, considering this solution is in practice the most efficient one (and most arguments against it come down to habits or personal preferences). If not then we'll always go for a lesser solution that never completely works. |
It will also require an extra tap on the hamburger menu to bring up the buffer list, which will contain the |
2 clicks to close a channel, which, let's be honest, is not something people do every day. I really don't see the overkill. But whatever. Way too much bikeshedding here, I'd rather do something meaningful. It's pretty clear to me that there will be no consensus on the subject, for something as small and insignificant than an action that gets triggered on rare occasions. |
Good. Just today I accidentally pressed that huge |
FYI, considering that adding the context menu and changing design of the Send button are changes introduced in the next release, the next release should really address this. My take on this would be to merge as is. It is very possible that another context menu, more visible (for example, through a button where the current Leave/Close button lives), duplicating entries of the right click, will appear in the future. In such case, we can revamp the Leave/Close button, making it a 2-click action (yes it's a good thing). Keep in mind that closing a chat today deletes all scrollback from the app, it's very destructive. We should make it hard to close a channel at the moment, until we provide scrollback persistence and/or more actions. My definitive 👍 for this PR, and things aren't set in stone, we evolve quickly :-) |
Considering my only concern left with this PR is that without the Leave button, it's unclear how to close channels on mobile without opening the sidebar twice, I went ahead and added a button that adds a shortcut to the context menu. Can we add this patch to the PR? With that I think we'll have addressed pretty much the concerns of everyone. |
I'd be OK with that. What do you think, @xPaw? |
63df54a
to
a279f43
Compare
Fine with me, @maxpoulin64 feel free to commit your patch into this PR. |
I patched in @maxpoulin64 changes. |
@xPaw welp beat me to it. I'm finally giving my 👍 to this! 👏 |
Alright, a couple of thoughts before merging this:
Apart from that, I think the visual design of context menu can be improved, but definitely not in this PR. Great job and discussions, guys!! |
@astorije Positioning fixed. |
Everything works great, thanks all for this work. Merging now. |
Change close button behaviour
Closes #61, implements what was suggested in #61 (comment)