Skip to content
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

Merged
merged 3 commits into from
Mar 20, 2016
Merged

Change close button behaviour #184

merged 3 commits into from
Mar 20, 2016

Conversation

xPaw
Copy link
Member

@xPaw xPaw commented Mar 11, 2016

Closes #61, implements what was suggested in #61 (comment)

@xPaw xPaw added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Mar 11, 2016
@astorije astorije self-assigned this Mar 12, 2016
@astorije
Copy link
Member

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:

close_btn

@xPaw xPaw force-pushed the xpaw/close-button branch 2 times, most recently from 1bc898d to a279f43 Compare March 13, 2016 08:57
@xPaw
Copy link
Member Author

xPaw commented Mar 13, 2016

@astorije fixed.

background-color: rgba(0, 0, 0, .1);
opacity: .7 !important;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One less !important, yay!

@astorije
Copy link
Member

Very cool @xPaw, 👍!

@maxpoulin64
Copy link
Member

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?

@dgw
Copy link
Contributor

dgw commented Mar 15, 2016

That's the only way I close PMs on mobile, so if it is removed I would vote for hiding it on desktop only.

@maxpoulin64
Copy link
Member

@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.

@astorije
Copy link
Member

Do we have a specific reason why we want to remove that one?

Yes, please read #61 (comment) have some details about it.
In a nutshell, it is removing an inconsistent element of the interface: none of the buttons left in the UI have that design. In terms of UX, @xPaw's solution brings together the actions of opening or closing a channel, it is not at the other end of the app like it is currently. This is also in the same area than where the context menu lives, so here again, consistency.
I could go on but simply put, these buttons need to go :-)

That's the only way I close PMs on mobile, so if it is removed I would vote for hiding it on desktop only.

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.

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.

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.

@dgw
Copy link
Contributor

dgw commented Mar 15, 2016

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.

It will also require an extra tap on the hamburger menu to bring up the buffer list, which will contain the × on the active buffer. I'm sure we'll get used to it, but it is an extra tap. Using the long-press menu will also require two taps (one long on the buffer's entry in the list, one short on the menu item)—on top of opening the sidebar by tapping the hamburger.

@astorije
Copy link
Member

It will also require an extra tap on the hamburger menu to bring up the buffer list, which will contain the × on the active buffer. I'm sure we'll get used to it, but it is an extra tap. Using the long-press menu will also require two taps (one long on the buffer's entry in the list, one short on the menu item)—on top of opening the sidebar by tapping the hamburger.

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.
Do me a favor and at least get rid of the ugly design for this button (talking about the design, not the button per se), to match the rest of the application, and I'll close my eyes to pretend I'm happy with whatever solution.

@xPaw
Copy link
Member Author

xPaw commented Mar 15, 2016

It will also require an extra tap on the hamburger menu to bring up the buffer list, which will contain the × on the active buffer. I'm sure we'll get used to it, but it is an extra tap

Good. Just today I accidentally pressed that huge Leave button while on mobile. It really shouldn't be stuck there at all times.

@astorije
Copy link
Member

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 :-)

@maxpoulin64
Copy link
Member

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.

For the visual:
capture d ecran_2016-03-19_15-15-59
capture d ecran_2016-03-19_15-16-11

@astorije
Copy link
Member

I'd be OK with that. What do you think, @xPaw?

@xPaw
Copy link
Member Author

xPaw commented Mar 20, 2016

Fine with me, @maxpoulin64 feel free to commit your patch into this PR.

@xPaw
Copy link
Member Author

xPaw commented Mar 20, 2016

I patched in @maxpoulin64 changes.

@maxpoulin64
Copy link
Member

@xPaw welp beat me to it. I'm finally giving my 👍 to this! 👏

@astorije
Copy link
Member

Alright, a couple of thoughts before merging this:

  • Now that we have @maxpoulin64's menu, is keeping the × button in the sidebar relevant? It means we now have 3 ways to close a chat: 2 in the active one (× and dropdown menu) and 1 in inactive channels (right-click menu). In comparison, we have only one way to open a new chat from the UI: clicking on a channel someone mentioned. I know I'm backtracking a bit here, but things evolve, and I don't think it's necessary to have the sidebar element anymore. I'll leave it up to you.

  • The dropdown menu gets positioned depending on where the button was clicked:
    position-dropdown

    Is it easily possible to have it opened always at the same position no matter where the button gets clicked? If not easily doable, perfectly fine if it goes in a further PR.

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!!

@xPaw
Copy link
Member Author

xPaw commented Mar 20, 2016

@astorije Positioning fixed.

@astorije
Copy link
Member

Everything works great, thanks all for this work. Merging now.

astorije added a commit that referenced this pull request Mar 20, 2016
@astorije astorije merged commit 52708b2 into master Mar 20, 2016
@astorije astorije deleted the xpaw/close-button branch March 20, 2016 21:21
@astorije astorije added this to the 1.4.0 milestone Oct 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Tickets that describe a desired feature or PRs that add them to the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants