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

[Hold for Payment 1st July] Shortcut Keys - There is no way to start a new group chat using shortcut keys #3349

Closed
isagoico opened this issue Jun 3, 2021 · 33 comments
Assignees
Labels
Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor

Comments

@isagoico
Copy link

isagoico commented Jun 3, 2021

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Deliverables

  • Implement a shortcut key that opens the New Group page
  • The keyboard shortcut we'd like to use is CMD + SHIFT + K

Platform:

Web ✔️
iOS
Android
Desktop App ✔️
Mobile Web

Notes/Photos/Videos:

Originally raised by @mallenexpensify here: https://expensify.slack.com/archives/C01GTK53T8Q/p1622734995479600

There is no way to start a new group chat using shortcut keys (or quickly via keyboard-only)

Some context on the decision to use this particular shortcut can be found here in the #expensify-open-source Slack channel.

Upwork Link: https://www.upwork.com/jobs/~012a19716ca69fca51


View all open jobs on Upwork

@MelvinBot
Copy link

Triggered auto assignment to @Dal-Papa (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@rushatgabhane
Copy link
Member

Hi, can I pick up this issue?

@mallenexpensify
Copy link
Contributor

Hi @rushatgabhane if you're interested in fixing the issue, please propose a solution for an engineer to review.

@rushatgabhane

This comment has been minimized.

@mallenexpensify
Copy link
Contributor

Dropped a note in here for feedback, feel free to chime in there too @rushatgabhane . https://expensify.slack.com/archives/C01GTK53T8Q/p1622819702017500?thread_ts=1621969175.208000&cid=C01GTK53T8Q

@roryabraham
Copy link
Contributor

I can confirm this can be worked on externally, adding the external label now

@roryabraham roryabraham added the External Added to denote the issue can be worked on by a contributor label Jun 8, 2021
@MelvinBot
Copy link

Triggered auto assignment to @trjExpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@trjExpensify
Copy link
Contributor

Did we get consensus on what keyboard shortcut to use here? Seems like a couple were floated in that thread on accessibility, but not very widely discussed. Are we implementing one for New Chat somewhere else, or do we only want one for New Group. what's the reasoning for that?

@mallenexpensify, I see you raised this issue originally in #expensify-open-source:

ISSUE: There is no way to start a new group chat using shortcut keys (or quickly via keyboard-only)

When raising these sorts of improvements in the channel, I think it would be helpful to propose what shortcut you think we should use and why, to promote a discussion in the same thread as to what we want implemented here. That way, when we create the issue there's a clearer direction of the high level deliverables.

@roryabraham
Copy link
Contributor

I thought we landed on CMD + SHIFT + K to create a new group, because CMD + K creates a new 1:1 chat.

@trjExpensify
Copy link
Contributor

CMD + K opens Search, not New Chat right? I don't believe we have a shortcut for New Chat.

@rushatgabhane
Copy link
Member

rushatgabhane commented Jun 9, 2021

CMD + K opens Search, not New Chat right? I don't believe we have a shortcut for New Chat.

Yeah you're right, but you can also start a new chat from search. At least by searching email or phone.

@trjExpensify
Copy link
Contributor

Totally, just trying to confirm that there isn't another shortcut in use/in the works for New Chat that could make more sense to couple with New Group vs with chat switching (Search).

@mallenexpensify
Copy link
Contributor

Regardless of what it is, it's just opening the RHP in the same fashion that clicking the green plus icon then 'new group chat'. I (kinda/sorta) tested earlier today and CMD + SHIFT + K made (enough) sense to me. We could raise again in #expensify-open-source if we think it would be helpful. (sorry to not be more helpful or definitive and.. for all the (()))

@trjExpensify
Copy link
Contributor

Cool, so it doesn't sound like we've got anything for New Chat coming and I couldn't find an issue for that, so let's proceed with CMD + SHIFT + K. I'll add the deliverables to the OP, and get it onto Upwork now, so @rushatgabhane can apply 👍

@trjExpensify
Copy link
Contributor

@mallenexpensify
Copy link
Contributor

@rushatgabhane are you interested in this job? If so, can you propose a fix here? One the proposal is accepted we'll hire in Upwork.

(@trjExpensify should the Exported label be added now to assign an engineer for review? I think so....)

@MelvinBot
Copy link

Triggered auto assignment to @Beamanator (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@rushatgabhane
Copy link
Member

I'm currently working on another job, so I don't qualify to work as I'm a new contributor.
Please feel free to take this one.

@Beamanator
Copy link
Contributor

Thanks @rushatgabhane 👍 If you finish your other job, feel free to come back and submit a proposal for the new Command + Shift + K shortcut here :)

@mallenexpensify
Copy link
Contributor

@rushatgabhane it appears your other PR is almost merged. If you'd like to propose a solution for this issue here, and we accept, we can assign this to you and you can submit the PR for it once the other issue is closed.

@rushatgabhane
Copy link
Member

Slack thread

@Beamanator
Copy link
Contributor

Hey @rushatgabhane thanks for doing that research in the linked slack thread - It sounds like we're going to adjust the issue so the shortcuts will be OS-specific - mainly, on Mac we'll use "Command + ..." and Windows we'll use "Control + ...". Are you willing to submit a proposal now or would you prefer waiting for a few more comments in the thread you linked?

@rushatgabhane

This comment has been minimized.

@Beamanator
Copy link
Contributor

Beamanator commented Jun 24, 2021

Hey @rushatgabhane Thanks for your investigation! Sorry for the super slow reply, I just got back from vacation!

Well, I tried. I think the way KeyboardShortcut.subscribe is implemented will need to change drastically.
...
Becuase this triggers on Command + K, which is fine but it also triggers on AnyKeyCombination + Command + K.

This is a great point, I'm not sure we'd tested that out before. Could you bring this up as a new discussion in #expensify-open-source? I think we'll need some other people to add some thoughts on how useful it is to fix that (for the record, I think it would definitely be useful to fix that)

Windows Chrome

Assume KeyboardShortcut.subscribe('K', ['control'])

This doesn't work at all, chrome selects address bar.
event.preventDefault() isn't working for some reason.

I don't have a windows machine to test this (sadly), but I'm surprised setting a shortcut with KeyboardShortcut.subscribe(... doesn't override typical Chrome shortcuts for you -> If I change 'K' to 'S' (so the app opens the search page on Command + S instead of Command + K), I am seeing the search page open instead of Chrome's typical "Save as" menu -> Are you seeing that? (Note: as you probably guessed, I'm developing on a Mac)

@rdjuric
Copy link
Contributor

rdjuric commented Jun 24, 2021

Just to add my perspective on this:

Becuase this triggers on Command + K, which is fine but it also triggers on AnyKeyCombination + Command + K.

This looks fine to me, I think it's how shortcuts work everywhere. On macOS I can open the Spotlight with CMD + Space but adding any key to the combination CMD + Any Key + Space will still open the Spotlight (if the new combination doesn't match another shortcut, for sure). Tested this on other applications and got the same result.

This doesn't work at all, chrome selects address bar.
event.preventDefault() isn't working for some reason.

Are we sure we're not exiting before the event.preventDefault()? I think it's only called when there's a event/callback associated with that keypress already

@rushatgabhane
Copy link
Member

rushatgabhane commented Jun 24, 2021

This looks fine to me, I think it's how shortcuts work everywhere. On macOS I can open the Spotlight with CMD + Space but adding any key to the combination CMD + Any Key + Space will still open the Spotlight (if the new combination doesn't match another shortcut, for sure). Tested this on other applications and got the same result.

You're right! That provides me some insight on how shortcuts work.
Only exception is anyModifier + Command + K. Even if that shortcut doesn't exist.

  • Cltr + S save. Cltr + Atl + S does nothing.
  • Win + D shows desktop. Win + Shift + D does nothing.


I am seeing the search page open instead of Chrome's typical "Save as" menu -> Are you seeing that?

Ran a clean test. I get the search page for control + K.
My bad, event.preventDefault() does work.

Of course, dedicated OS shortcuts won't be overridden. Hence the need for OS specific shortcuts :)
eg: Win + K which cant be overridden.

@rushatgabhane
Copy link
Member

rushatgabhane commented Jun 24, 2021

@Beamanator with some new insights,
I have a working solution and I wanna be assigned to this job.

This is how shortcuts are implemented I think.
anyNonModifier + Shortcut is fine, but extraModifier + Shortcut shouldn't trigger that event.

Slack discussion for this

Proposal

Basically, the event will be triggered only if the modifiers match EXACTLY.
eg: Alt+ Command + K won't open search.
OS specific shortcuts using libs/openOpertingSystem

Loop over all eventCallbacks , instead of just checking for last callback.
https://github.com/Expensify/Expensify.cash/blob/655f9efd75a8357ef2eca1aa847e84ae075dfb3b/src/libs/KeyboardShortcut/index.js#L15-L16

Demonstration

Expensify.cash.-.Google.Chrome.2021-06-25.13-01-15.mp4

@Beamanator
Copy link
Contributor

@rushatgabhane I like your proposal to fix shortcuts from being run if extra modifiers are pressed!

I do want to note that this issue is mainly about the new shortcut group CMD + SHIFT + K to open a Group Chat (at least on MacOS), so once you confirm that you'll do that as well (and probably replace CMD with Control on Windows), I'll assign this to you!

@rushatgabhane
Copy link
Member

rushatgabhane commented Jun 25, 2021

@Beamanator Haha yes, I've posted a demo for just that in my prev comment.
OS specific shortcuts to open new group chat, search.

@Beamanator
Copy link
Contributor

Aah thanks very much @rushatgabhane , I didn't see you mention the shortcut specifically for "group chats" in the text, and I missed that the second modal you opened in your video was group chat!

Thanks for sticking with me, please submit a PR when you have a chance! And @trjExpensify please hire @rushatgabhane on Upwork when you have a chance 👍

@rushatgabhane
Copy link
Member

This issue can be closed. PR merged.
Also, can someone hire me on upwork :)

@trjExpensify trjExpensify changed the title Shortcut Keys - There is no way to start a new group chat using shortcut keys [Hold for Payment 1st July] Shortcut Keys - There is no way to start a new group chat using shortcut keys Jun 29, 2021
@trjExpensify
Copy link
Contributor

Thanks, @rushatgabhane. We'll keep the issue open for 7 days and then close it once paid. 👍

Re: hiring on Upwork, I'm just working through a change to our Upwork account over the weekend. I'll hire you as soon as I can, likely when the US get online later and that is resolved - apologies for the delay! (Expensify folks, thread here).

@trjExpensify
Copy link
Contributor

Completed the contract on Upwork. Thanks, @rushatgabhane!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

9 participants