deck icon indicating copy to clipboard operation
deck copied to clipboard

feat: #46 add shortcut function

Open oneWaveAdrian opened this issue 3 years ago • 12 comments

Signed-off-by: Adrian Missy [email protected]

  • Resolves: #46
  • Target version: master

Summary

TODO

  • [x] base card shortcut framework
  • [x] base board shortcut framework
  • [x] add Trello inspired card shortcuts preset
    • [x] spacebar un-/assigns currentUser
    • [x] enter opens card
    • [x] d set due date
    • [x] a,m assign any member
    • [x] c archive card
    • [x] l add tag/label to card
    • [x] t edit card title
  • [x] add Trello inspired board shortcuts preset
    • [x] n create new card
    • [x] / focus on search field
    • [x] q filter cards by currentUser
    • [x] f open filter menu
    • [x] x clear all filters
  • [x] update documentation with shortcuts

OPTIONAL

  • [ ] dynamic key preset definitions in settings to enable custom shortcut definition

Checklist

  • [x] Code is properly formatted
  • [x] Sign-off message is added to all commits
  • [ ] Tests (unit, integration, api and/or acceptance) are included
  • [x] Documentation (manuals or wiki) has been updated or is not required

oneWaveAdrian avatar Apr 27 '22 14:04 oneWaveAdrian

@juliushaertl I realized that a newly created card gets an order value of 999. This prevents the shortcut modals from being displayed correctly. I countered it for now by displaying the modal in the screen center. This is not ideal. Is there a specific reason why 999 is set for a new card other than easily placing it at the end of a stack? Is there no dynamic way of placing it at the end of the stack?

oneWaveAdrian avatar Apr 29 '22 16:04 oneWaveAdrian

@juliushaertl In regards to settings I don't know how you guys would want a dynamic implementation. I got the feeling you want to keep the amount of settings low. Therefore I marked it as optional. If dynamic key definition is not required this is ready to merge. Otherwise please advise on best practices for creating the settings dialog.

oneWaveAdrian avatar Apr 29 '22 22:04 oneWaveAdrian

Kindly requesting review of this PR.

oneWaveAdrian avatar May 02 '22 17:05 oneWaveAdrian

@juliushaertl I would really need this feature myself - when do you think a review can start?

oneWaveAdrian avatar May 05 '22 12:05 oneWaveAdrian

Does anybody have time to review this in the near future? @stefan-niedermann @juliushaertl? I would really like to see this in the next release as there is an urgent need within my team for it. Thanks a bunch! Unfortunately I can not change the status to review needed and the only way to get anybody's attention is to guess via comments who might be responsible/able to look at it, which is a little frustrating. So sorry if I approached the wrong guys, it's just hard to understand how the desired workflow is.

oneWaveAdrian avatar May 09 '22 16:05 oneWaveAdrian

As this drags on I will resolve conflicts after a successful review. Don't want to keep applying changes for another two weeks...

oneWaveAdrian avatar May 11 '22 15:05 oneWaveAdrian

It's been almost a month since I submitted this PR - what is the typical time to expect a review/feedback around here @stefan-niedermann @juliushaertl? I am sure you have lots to do, a rough timeframe is perfectly fine by me. Maybe this is something that needs to be mentioned in the readme as well to reduce the amount of requests to you?

oneWaveAdrian avatar May 24 '22 15:05 oneWaveAdrian

@stefan-niedermann @juliushaertl @oneWaveAdrian My company could potentially sponsor this if that helps move this PR forward. Is there some kind of intake procedure for sponsorship you could point me to?

DrFriedParts avatar Jun 29 '22 20:06 DrFriedParts

@stefan-niedermann @juliushaertl @oneWaveAdrian My company could potentially sponsor this if that helps move this PR forward. Is there some kind of intake procedure for sponsorship you could point me to?

@DrFriedParts I am basically "just" waiting for an approval/review from the Nextcloud team, still willing to add the rest of the shortcuts.

oneWaveAdrian avatar Jun 29 '22 20:06 oneWaveAdrian

@juliushaertl is this ever going to be reviewed? It saddens me that I put quite some work into this and it is basically ignored, absolutely no feedback from the team on a solution of a requested topic.

oneWaveAdrian avatar Jul 08 '22 20:07 oneWaveAdrian

Bumping after almost another month of inactivity. @juliushaertl would you please take a look at this? I'm not giving up on this one, too much work has gone into it.😡

oneWaveAdrian avatar Aug 03 '22 14:08 oneWaveAdrian

Hi @oneWaveAdrian I'm very sorry for the long silence here. I highly appreciate your contribution and I've not forgotten about this, just my GitHub notification backlog is quite large. I've scheduled some time for this week to give it an in depth review.

I realized that a newly created card gets an order value of 999. This prevents the shortcut modals from being displayed correctly. I countered it for now by displaying the modal in the screen center. This is not ideal. Is there a specific reason why 999 is set for a new card other than easily placing it at the end of a stack? Is there no dynamic way of placing it at the end of the stack?

Yes, it is mainly about putting the card at the end of the list. I guess we could also apply the correct order value on card creation by taking the max value of order and increasing that by 1 in the backend.

dynamic key preset definitions in settings to enable custom shortcut definition

We usually aim for having good defaults instead of highly customisable settings, so I'd agree that this is not necessary. From the shortcut overview this definitely looks like a sane choice of shortcuts.

juliusknorr avatar Aug 09 '22 05:08 juliusknorr

@luka-nextcloud @juliushaertl what's the status on this PR? I didn't get any answers to my questions - can we merge this one as well?

oneWaveAdrian avatar Oct 26 '22 12:10 oneWaveAdrian

@luka-nextcloud @juliushaertl what's the status on this PR? I didn't get any answers to my questions - can we merge this one as well?

@juliushaertl ...?

oneWaveAdrian avatar Oct 31 '22 19:10 oneWaveAdrian

@oneWaveAdrian Sorry for the late replies. Currently @marcelklehr and I are catching up on all the open pull requests and we lined this up for review. I'll get to that hopefully during this week. Could you maybe rebase your changes already to the latest master branch to get the conflicts resolved?

juliusknorr avatar Oct 31 '22 19:10 juliusknorr

Just one thing before getting into the review, from an accessibility perspective single key shortcuts would be problematic. Therefore at least turning it off should be supported:

A global option to disable keyboard shortcuts was added to the accessibility settings. Since it heavily depends on the screenreader and tools that you use if Ctrl and/or Alt or other things are okay to use or not and maintaining a more detailed list is too much effort, we went for a global on/off switch. Apps can use this public javascript API call to determine whether the user used the opt-out: OCP.Accessibility.disableKeyboardShortcuts(). If that is the case, no additional shortcuts shall be registered by any app. Only space to toggle checkboxes and enter to submit the currently active buttons/links are okay: https://github.com/nextcloud/server/pull/34081

juliusknorr avatar Nov 08 '22 10:11 juliusknorr

I think it would be beneficial to use something like https://github.com/fgr-araujo/vue-shortkey especially to avoid hitting shortcuts in input elements. The mail app uses an event bus to avoid having to pass props around for this: https://github.com/nextcloud/mail/blob/c36f8f597e2c390a102061279b752db58875edac/src/components/Mailbox.vue#L183 https://github.com/nextcloud/mail/blob/c36f8f597e2c390a102061279b752db58875edac/src/components/MailboxThread.vue#L129

marcelklehr avatar Dec 01 '22 12:12 marcelklehr

I think it would be beneficial to use something like https://github.com/fgr-araujo/vue-shortkey especially to avoid hitting shortcuts in input elements.

I understand your point, but i would highly recommend against yet another (almost) unmaintained dependency with lacking vue3 support. It will be a huge pain to upgrade deck to vue3 as is.. just my 2cents.

shoetten avatar Dec 01 '22 15:12 shoetten

Hey @shoetten

~~Glad you're still active on this and sorry for the long wait!~~ Oops, I thought, you were the OP :see_no_evil:

yet another (almost) unmaintained dependency with lacking vue3 support

Mh, that's a good point. Let's avoid adding a new dependency then. Do you think using an event bus (maybe even https://www.npmjs.com/package/@nextcloud/event-bus) would be useful?

marcelklehr avatar Dec 01 '22 16:12 marcelklehr

Am still active in this as well @marcelklehr, just waiting for consensus to get started, before I spend precious time developing, just so I can wait another 8mos for feedback so my code can be deprecated before it was even reviewed...

oneWaveAdrian avatar Dec 13 '22 15:12 oneWaveAdrian

just so I can wait another 8mos for feedback so my code can be deprecated before it was even reviewed...

I know it must be frustrating. I'm sorry you had to wait so long. We're trying to get better with timely reviews and encouraging community contributions. :hearts:

marcelklehr avatar Dec 13 '22 15:12 marcelklehr

Resolved Conflicts (not too easy after a year) – anything else that needs to be done here or can we get this thing merged?

oneWaveAdrian avatar Apr 06 '23 00:04 oneWaveAdrian

Anybody? @marcelklehr @juliushaertl

For the record I'm not even using Nextcloud anymore - just want to get it done for the community and not have wasted my time...

oneWaveAdrian avatar Apr 28 '23 20:04 oneWaveAdrian

I did a rebase of this against latest main branch and started with some adjustments, some still left but I'm happy to continue to get this merged:

  • [x] Respect keyboard shortcut accessibility setting
  • [x] Move to event bus for propagating shortcut actions across components
  • [x] Move components to new Nc* naming
  • [ ] ShortCutWidgets.vue seems non-functional, needs migration to NcSelect
  • [ ] Fix Ctrl-f double shortcut for search
  • [ ] Figure out where to put the initial focus, as right now shortcuts are only catched when the Board component is in focus
  • [ ] Add arrow navigation between cards
  • [ ] Add help modal to show shortcuts (like in text/talk/mail)
  • [ ] Discuss shortcuts with design team
  • [ ] Update docs

juliusknorr avatar Oct 18 '23 19:10 juliusknorr

/backport to stable28

juliusknorr avatar Dec 07 '23 15:12 juliusknorr

Yes! Thanks to @oneWaveAdrian for the groundwork! The next release of deck is shaping out to be an awesome one.

shoetten avatar Dec 07 '23 15:12 shoetten

Definitely. Thanks a ton @oneWaveAdrian and I'm really sorry to have taken so long to pick this up and get merged. Looking much forward to use that myself 🎉

juliusknorr avatar Dec 07 '23 15:12 juliusknorr