Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Add keyboard shortcut to close the current conversation #5253

Merged
merged 3 commits into from
Nov 18, 2020
Merged

Add keyboard shortcut to close the current conversation #5253

merged 3 commits into from
Nov 18, 2020

Conversation

miterion
Copy link
Contributor

I personally prefer to not have any conversation open if I am not currently chatting with a user or a room, since it might lead to accidental "reading" a message without me noticing it. Many messengers like telegram and Signal support this by allowing the user to close the current conversation and showing an empty panel instead. Element web does something similarly on the first login where it just shows a welcome screen. This PR adds a keyboard shortcut to trigger showing this welcome screen without needing to login again or manually navigating to the #/home page.

I chose the Ctrl+, here quite arbitrarily, feel free to change it to something different if you like.

The new shortcut in the overview:
image

@t3chguy
Copy link
Member

t3chguy commented Sep 27, 2020

Given Ctrl+. being right panel, Ctrl+, feels like it should be left Panel.

Chrome has Alt + Home for going Home, maybe that's a good convention

@miterion
Copy link
Contributor Author

miterion commented Sep 27, 2020

Given Ctrl+. being right panel, Ctrl+, feels like it should be left Panel.
Chrome has Alt + Home for going Home, maybe that's a good convention

I agree that , might be confusing for users, but overriding something like Alt+Home might break common usage patterns (I also do not have a home key on my thinkpad keyboard :/).
What do you think about a more complicated pattern like for example Ctrl + Shift + Q?

@orangecms
Copy link
Contributor

How do you feel about Ctrl + W? It is common for closing the current tab in GUI apps.

@t3chguy
Copy link
Member

t3chguy commented Sep 29, 2020

I'd expect much smarter behaviour there then, where it closes the active modal first, if one is open otherwise "closes the room" (goes home)

@miterion
Copy link
Contributor Author

miterion commented Oct 5, 2020

I'd expect much smarter behaviour there then, where it closes the active modal first, if one is open otherwise "closes the room" (goes home)

I think that is a bit out of scope for my meager JavaScript knowledge. If you think that is necessary I would suggest that I close this PR and create an issue in the tracker.

@t3chguy
Copy link
Member

t3chguy commented Oct 5, 2020

Well if you want it to be Ctrl + W which is a very common close current thing then it has to be smarter
If you want something more akin to "Go Home" then that can be much simpler but shouldn't overload an existing competing keyboard shortcut

@miterion
Copy link
Contributor Author

miterion commented Oct 5, 2020

I changed it now to Ctrl+Alt+H which is not used afaik. Is that better in your opinion?

image

@t3chguy
Copy link
Member

t3chguy commented Oct 5, 2020

Much better, couple of nits:

On Mac Ctrl is rarely used, in that scenario Cmd should probably be used instead, we have CMD_OR_CTRL and other helpers for this.

The copy could probably be better as it'd apply to more than just conversations, e.g Community Views too. Should be more general, maybe being more about where you're going i.e Home.

@miterion
Copy link
Contributor Author

miterion commented Oct 5, 2020

On Mac Ctrl is rarely used, in that scenario Cmd should probably be used instead, we have CMD_OR_CTRL and other helpers for this.

Think I found a fix for that.

The copy could probably be better as it'd apply to more than just conversations, e.g Community Views too. Should be more general, maybe being more about where you're going i.e Home.

What do you mean by copy?

@t3chguy
Copy link
Member

t3chguy commented Oct 5, 2020

copy

Text - see https://en.wikipedia.org/wiki/Copy_(written)

@miterion
Copy link
Contributor Author

miterion commented Oct 5, 2020

Text - see https://en.wikipedia.org/wiki/Copy_(written)

Yeah, I could have guessed that 🤦

These are some suggestions from the top of my mind:

  1. Open Home View/Page
  2. Go to Home View

@t3chguy
Copy link
Member

t3chguy commented Oct 6, 2020

The latter seems sane to me

@t3chguy
Copy link
Member

t3chguy commented Oct 6, 2020

@t3chguy t3chguy requested review from a team October 6, 2020 07:40
@turt2live turt2live added the Z-Community-PR Issue is solved by a community member's PR label Oct 6, 2020
Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

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

Code wise it looks sane, I'd consider having it close all open Modal too otherwise the user might feel like its broken if they have Settings open

turt2live
turt2live previously approved these changes Oct 7, 2020
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

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

just leaving a checkmark so github stops being confused and can instead be differently confused

@miterion
Copy link
Contributor Author

Code wise it looks sane, I'd consider having it close all open Modal too otherwise the user might feel like its broken if they have Settings open

I added this feature in 40650bb but it only works after a user refocused the view (at least in my browser). This also happens with the Ctrl+/ shortcut, so it is not limited to my code.

@t3chguy t3chguy requested a review from turt2live October 13, 2020 17:17
src/components/structures/LoggedInView.tsx Outdated Show resolved Hide resolved
src/Modal.tsx Outdated Show resolved Hide resolved
@turt2live turt2live requested review from t3chguy and removed request for turt2live October 14, 2020 22:36
@turt2live turt2live dismissed their stale review October 14, 2020 22:36

was only a placeholder review

@turt2live
Copy link
Member

@miterion in general, please avoid force-pushing as it makes review harder.

@miterion
Copy link
Contributor Author

@miterion in general, please avoid force-pushing as it makes review harder.

Will do in the future, sorry for the fuss

@turt2live turt2live merged commit e4c7ab5 into matrix-org:develop Nov 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants