Skip to content

Comments

[NEW] Switch channel with keyboard shortcuts#10366

Open
cmrd-senya wants to merge 2 commits intoRocketChat:developfrom
cmrd-senya:4256-keyboard-channel-switch
Open

[NEW] Switch channel with keyboard shortcuts#10366
cmrd-senya wants to merge 2 commits intoRocketChat:developfrom
cmrd-senya:4256-keyboard-channel-switch

Conversation

@cmrd-senya
Copy link

@cmrd-senya cmrd-senya commented Apr 6, 2018

@RocketChat/core

Closes #4256

This adds support for switching channels using keyboard shortcuts. I'm expecting this not to be a final version and I expect the code to change according with the community feedback.

In the present version I supported two pairs of combinations: alt + up/alt + down and ctrl + tab/ctrl + shift + tab. The latter is conflicting with the browser hotkeys for switching tab, but I think it is useful for the users of the desktop version because it is consistent with how other desktop chat clients work. But if support ctrl + (shift) + tab we have to block them on the web version then. I don't know how is it possible to achieve.

Anyway, I think I need some feedback since it is my first PR for Rocket.Chat and it is the first time I work with Meteor, so I'm not too confident about the code.

Copy link
Contributor

@graywolf336 graywolf336 left a comment

Choose a reason for hiding this comment

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

Can you fix the linting issues? That way we can review this better?

@cmrd-senya cmrd-senya force-pushed the 4256-keyboard-channel-switch branch from 32df854 to 5b492a4 Compare April 9, 2018 08:01
@RocketChat RocketChat deleted a comment Apr 9, 2018
@RocketChat RocketChat deleted a comment Apr 9, 2018
@RocketChat RocketChat deleted a comment Apr 9, 2018
@RocketChat RocketChat deleted a comment Apr 9, 2018
@RocketChat RocketChat deleted a comment Apr 9, 2018
@RocketChat RocketChat deleted a comment Apr 9, 2018
@RocketChat RocketChat deleted a comment Apr 9, 2018
@RocketChat RocketChat deleted a comment Apr 9, 2018
@RocketChat RocketChat deleted a comment Apr 9, 2018
@cmrd-senya
Copy link
Author

Updated with lint issues fixed.

@iprok
Copy link

iprok commented Jun 23, 2018

Anything else blocking merging this to the rocket chat release? It's highly requested feature for us.

vynmera
vynmera previously approved these changes Jun 23, 2018
@cmrd-senya
Copy link
Author

Please change status to "review required", because requested changes were commited

@vynmera
Copy link
Contributor

vynmera commented Jul 17, 2018

@graywolf336

@iprok
Copy link

iprok commented Aug 5, 2018

@graywolf336 any problem with this request? Would like to see in the next release

@ggazzo ggazzo added the area: ui Touches the code on client side label Aug 6, 2018
@ggazzo ggazzo added this to the 0.69.0 milestone Aug 6, 2018
@tassoevan tassoevan dismissed graywolf336’s stale review August 20, 2018 16:52

I'm dismissing this review since the requested changes were made.

@RocketChat RocketChat deleted a comment Aug 20, 2018
@tassoevan
Copy link
Contributor

@ggazzo Can you check all that __helpers usage? I'm not sure if it safe to use them, I don't think it's a strong reason to block this PR though.

Copy link
Member

@ggazzo ggazzo left a comment

Choose a reason for hiding this comment

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

@cmrd-senya we dont use __helper, usually we create a common function to use cross templates.
ps: use RocketChat.roomTypes.openRouteLink(nextRoom.t, nextRoom); instead of

const name = RocketChat.roomTypes.getRouteLink(nextRoom.t, nextRoom);
FlowRouter.go(name);

any problem ask, @tassoevan will help you :)

@cmrd-senya
Copy link
Author

@cmrd-senya we dont use __helper, usually we create a common function to use cross templates.

@ggazzo, where do you normally put the common functions?

@cmrd-senya cmrd-senya force-pushed the 4256-keyboard-channel-switch branch from 8b443de to 863021d Compare November 14, 2018 22:54
@cmrd-senya
Copy link
Author

@ggazzo, @tassoevan,
I have updated the PR. Now helper functions are extracted to a separate file and imported to templates and to the channel switching code.

Please review.

@cmrd-senya cmrd-senya force-pushed the 4256-keyboard-channel-switch branch from 9a0e219 to 2cc2e09 Compare November 15, 2018 14:43
@tassoevan
Copy link
Contributor

Added a commit encapsulating the event attachments into a function to be called on template creation, leaving the module scope just for declarations. Also, I'm going to merge #12564 first, avoiding a conflict when writing a documentation for this new shortcuts.

@cmrd-senya
Copy link
Author

Added a commit encapsulating the event attachments into a function to be called on template creation, leaving the module scope just for declarations.

Tested, works for me.

Also, I'm going to merge #12564 first, avoiding a conflict when writing a documentation for this new shortcuts.

So I'll have to add documentation to this PR after #12564 is merged?

@tassoevan
Copy link
Contributor

Don't worry, I am ready to change it. I'll just request your review when done.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ tassoevan
❌ Senya


Senya seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: ui Touches the code on client side community stat: conflict

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Keyboard shortcut to switch between channels

10 participants