vikunja
/
frontend
Archived
6
39
Fork 49
Code Issues 1 Pull Requests 4 Projects Releases 26 Activity

Added hotkeys for priority, delete and favorite on the TaskDetailView #3400

Merged
konrad merged 5 commits from primeapple/frontend:add-hotkeys-for-priority-and-delete into main 2023-05-10 09:14:08 +00:00
Conversation 3 Commits 5 Files Changed 3 +21 -3
primeapple commented 2023-04-19 12:43:49 +00:00
Contributor
Copy Link

I finally implemented some of the features from https://community.vikunja.io/t/more-hotkeys-in-the-ui/1096/9

It's my first PR here, tell me what you think!

I finally implemented some of the features from https://community.vikunja.io/t/more-hotkeys-in-the-ui/1096/9 It's my first PR here, tell me what you think!
🚀 1
primeapple added 1 commit 2023-04-19 12:43:51 +00:00
frederick commented 2023-04-19 12:48:35 +00:00
Member
Copy Link

Hi primeapple!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://3400-add-hotkeys-for-priority-and-del--vikunja-frontend-preview.netlify.app

You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/.

Have a nice day!

Beep boop, I'm a bot.

Hi primeapple! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://3400-add-hotkeys-for-priority-and-del--vikunja-frontend-preview.netlify.app You can use this url to view the changes live and test them out. You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/. Have a nice day! > Beep boop, I'm a bot.
primeapple reviewed 2023-04-19 13:20:21 +00:00
src/views/tasks/TaskDetailView.vue Outdated
@ -406,6 +407,7 @@
@click="toggleFavorite"
variant="secondary"
:icon="task.isFavorite ? 'star' : ['far', 'star']"
v-shortcut="'s'"
primeapple commented 2023-04-19 13:20:21 +00:00
Author
Contributor

The s is for star, since f is already used for files

The `s` is for star, since `f` is already used for `files`
dpschen commented 2023-04-20 14:31:07 +00:00
Member

I don't like s for files that much.

Didn't check, but would a for attachments still be free? I don't think that too many users use that shortcut (?).

I don't like `s` for files that much. Didn't check, but would `a` for **attachments** still be free? I don't think that _too_ many users use that shortcut (?).
konrad commented 2023-04-20 20:40:32 +00:00
Owner

a is used for assignees.

`a` is used for assignees.
dpschen commented 2023-04-20 21:38:14 +00:00
Member

How about using u (upload)

How about using `u` (upload)
primeapple commented 2023-04-21 10:41:17 +00:00
Author
Contributor

I honestly don't care what the hotkeys are. Since Vikunja supports imports from other services, I would just take their hotkeys.

But for now, maybe let's just implement it. I'd rather have bad hotkeys than no hotkeys.

The future would be something like having a "classic" hotkey layout, a "Todoist" inspired one and a custom one.

Are you fine with this?

I honestly don't care what the hotkeys are. Since Vikunja supports imports from other services, I would just take their hotkeys. But for now, maybe let's just implement it. I'd rather have bad hotkeys than no hotkeys. The future would be something like having a "classic" hotkey layout, a "Todoist" inspired one and a custom one. Are you fine with this?
👍 1
primeapple marked this conversation as resolved
primeapple reviewed 2023-04-19 13:21:01 +00:00
src/views/tasks/TaskDetailView.vue Outdated
@ -416,6 +418,7 @@
icon="trash-alt"
:shadow="false"
class="is-danger is-outlined has-no-border"
v-shortcut="'x'"
primeapple commented 2023-04-19 13:21:01 +00:00
Author
Contributor

x is eXterminate... Just kidding, thats the hotkey that is used for deletion of a single char in vim.

`x` is eXterminate... Just kidding, thats the hotkey that is used for deletion of a single char in vim.
dpschen commented 2023-04-20 14:28:35 +00:00
Member

How about meta + backspace instead?
I would use two keys by intend here, so that there has to be a bit more intend by the user in order to delete a task.

How about `meta` + `backspace` instead? I would use two keys by intend here, so that there has to be a bit more intend by the user in order to delete a task.
primeapple commented 2023-04-21 10:42:43 +00:00
Author
Contributor

I'd really avoid using modifier keys in the browser. It often interferes with browser specific behaviour.

Are you fine with this explanation or do you really want me to change it? :)

I'd really avoid using modifier keys in the browser. It often interferes with browser specific behaviour. Are you fine with this explanation or do you really want me to change it? :)
konrad commented 2023-04-24 10:57:20 +00:00
Owner

I think using a single key here is fine since there's a modal requiring a confirmation when deleting a task anyway.

I think using a single key here is fine since there's a modal requiring a confirmation when deleting a task anyway.
dpschen commented 2023-05-02 14:32:18 +00:00
Member

@primeapple I talked with @konrad about this. After some consideration we decided on using:

  • shift + delete in general
  • shift + backspace additionally for apple devices (if I'm not mistaken we have a detection helper somewhere)
@primeapple I talked with @konrad about this. After some consideration we decided on using: - `shift` + `delete` in general - `shift` + `backspace` additionally for apple devices (if I'm not mistaken we have a detection helper somewhere)
primeapple commented 2023-05-03 10:01:14 +00:00
Author
Contributor

Thanks for the comment.

I see that this is your repo and I'm just a contributor. Because of that I won't argue to much and accept it. But please please consider the following thoughts:

  • Delete is quite unergonomic to reach compared to the other hotkeys that all rely on a letter. Backspace is not perfect, but would be fine. Why shift as a modifier, tho? Why not CTRL?

  • Why would you differentiate Delete and Backspace based on the OS of the computer?

There are keyboards that don't have the delete key easily reachable (it's on a different layer). The Backspace is almost always there. The x is always there.

As a user I don't want to use obscure hotkeys that are not supported anywhere else. Todoist uses Backspace + CTRL, that's also the one used in Emacs.
Vim uses x or d.

Could you write the reasoning behind your thoughts? As @konrad said before:

I think using a single key here is fine since there's a modal requiring a confirmation when deleting a task anyway.

Thanks for the comment. I see that this is your repo and I'm just a contributor. Because of that I won't argue to much and accept it. But please please consider the following thoughts: - Delete is quite unergonomic to reach compared to the other hotkeys that all rely on a letter. Backspace is not perfect, but would be fine. Why shift as a modifier, tho? Why not `CTRL`? - Why would you differentiate Delete and Backspace based on the OS of the computer? There are keyboards that don't have the delete key easily reachable (it's on a different layer). The Backspace is almost always there. The `x` is always there. As a user I don't want to use obscure hotkeys that are not supported anywhere else. Todoist uses `Backspace` + `CTRL`, that's also the one used in Emacs. Vim uses `x` or `d`. Could you write the reasoning behind your thoughts? As @konrad said before: > I think using a single key here is fine since there's a modal requiring a confirmation when deleting a task anyway.
konrad commented 2023-05-06 16:49:12 +00:00
Owner

The problem with keyboard shortcuts is they are hard to change later because once we introduce them, people start to use them and after some time they're in their muscle memory.

Why would you differentiate Delete and Backspace based on the OS of the computer?

I think we should, because people are used to using different keys on different OSes. For example, creating a new folder in Finder on MacOS uses a different shortcut than creating a new folder in MS Explorer.
That's for another PR though. (Todoist does something similar)

Why shift as a modifier, tho? Why not CTRL?

Mostly because shift is not used by the OS already and really unlikely to conflict with something else. Todoist uses shift delete for windows and cmd delete on macos.

To move this PR forward, let's use shift + delete for this and introduce shift + backspace (or something different) for macos in a later PR.

The problem with keyboard shortcuts is they are hard to change later because once we introduce them, people start to use them and after some time they're in their muscle memory. > Why would you differentiate Delete and Backspace based on the OS of the computer? I think we should, because people are used to using different keys on different OSes. For example, creating a new folder in Finder on MacOS uses a different shortcut than creating a new folder in MS Explorer. That's for another PR though. (Todoist [does something similar](https://todoist.com/help/articles/keyboard-shortcuts)) > Why shift as a modifier, tho? Why not CTRL? Mostly because shift is not used by the OS already and really unlikely to conflict with something else. Todoist uses shift delete for windows and cmd delete on macos. --- To move this PR forward, let's use shift + delete for this and introduce shift + backspace (or something different) for macos in a later PR.
👍 1
primeapple commented 2023-05-09 19:08:05 +00:00
Author
Contributor

I think we should, because people are used to using different keys on different OSes. For example, creating a new folder in Finder on MacOS uses a different shortcut than creating a new folder in MS Explorer.

People are not used to that. They will just try to avoid OSes with other keymaps, that's part of the reasons Apples Eco system is so locking in.

I gave away my mac just because I hated switching between Cmd and Ctrl

However, I changed it. It works, but yeah... I'll not use this hotkey, I'm faster with clicking it, honestly. Maybe if we at least could avoid the confirmation dialog, that would be a start 😄

Let's see that we get customizable shortcuts going.

> I think we should, because people are used to using different keys on different OSes. For example, creating a new folder in Finder on MacOS uses a different shortcut than creating a new folder in MS Explorer. People are not used to that. They will just try to avoid OSes with other keymaps, that's part of the reasons Apples Eco system is so locking in. I gave away my mac just because I hated switching between `Cmd` and `Ctrl` However, I changed it. It works, but yeah... I'll not use this hotkey, I'm faster with clicking it, honestly. Maybe if we at least could avoid the confirmation dialog, that would be a start 😄 Let's see that we get customizable shortcuts going.
👍 1
dpschen requested changes 2023-04-20 14:41:29 +00:00
src/i18n/lang/de-DE.json Outdated
@ -894,7 +894,10 @@
"color": "Die Farbe dieser Aufgabe ändern",
"move": "Move this task to another project",
"reminder": "Erinnerungen für diese Aufgabe verwalten",
"description": "Aufgabenbeschreibung bearbeiten"
dpschen commented 2023-04-20 14:39:34 +00:00
Member

Translations (aka other languages than English) should happen on Crowdin.

Regarding translation [DE]:
Ich habe das Gefühl, dass diese Labels hier alle recht sperrig formuliert sind. Hat nichts mit deiner Übersetzung zu tun, denn du hast ja offensichtlich nur den Stil übertragen auf die neuen Übersetzungen. Bsp:

  • Die Farbe dieser Aufgabe ändern könnte entweder Aufgabenfarbe ändern sein. Oder sogar nur Farbe ändern, denn der Kontext im Dokument ist ja gegeben (?). Bin mir hier tatsächlich auch aus Accessability-Gründen unsicher, ob man hier den Kontext brauchen würde.
  • "Diese" würde ich entfernen bei "Diese Aufgabe löschen".
  • …
Translations (aka other languages than English) should happen on [Crowdin](https://crowdin.com/project/vikunja). Regarding translation \[DE\]: Ich habe das Gefühl, dass diese Labels hier alle recht sperrig formuliert sind. Hat nichts mit deiner Übersetzung zu tun, denn du hast ja offensichtlich nur den Stil übertragen auf die neuen Übersetzungen. Bsp: - `Die Farbe dieser Aufgabe ändern` könnte entweder `Aufgabenfarbe ändern` sein. Oder sogar nur `Farbe ändern`, denn der Kontext im Dokument ist ja gegeben (?). Bin mir hier tatsächlich auch aus Accessability-Gründen unsicher, ob man hier den Kontext brauchen würde. - "Diese" würde ich entfernen bei "Diese Aufgabe löschen". - …
✅ 1
konrad commented 2023-04-20 20:41:18 +00:00
Owner

Mach mal in Crowdin ne Discussion auf, hier gehört das nicht hin :)

Mach mal in Crowdin ne Discussion auf, hier gehört das nicht hin :)
👍 1 😆 1
primeapple commented 2023-04-21 10:44:22 +00:00
Author
Contributor

I changed "Diese Aufgabe löschen" zu "Aufgabe löschen". The other stuff I won't touch in this PR :D

I changed "Diese Aufgabe löschen" zu "Aufgabe löschen". The other stuff I won't touch in this PR :D
dpschen commented 2023-04-21 13:00:56 +00:00
Member

Translations (aka other languages than English) should happen on Crowdin.

What I meant by this is that you need to adjust this PR and remove any changes in this file. Because all translation changes other than English are coming from Crowdin. Best would be to use an interactive rebase.

After this branch has merged and Crowdin has synced we can then translate the string there.

> Translations (aka other languages than English) should happen on Crowdin. What I meant by this is that you need to adjust this PR and remove any changes in this file. Because all translation changes other than English are coming from Crowdin. Best would be to use an interactive rebase. After this branch has merged and Crowdin has synced we can then translate the string there.
primeapple commented 2023-04-21 18:44:49 +00:00
Author
Contributor

I see now.
Thanks for explaining!

I removed the changes from the german translation and forcepushed the branch! :)

I see now. Thanks for explaining! I removed the changes from the german translation and forcepushed the branch! :)
👍 2 ✅ 1
dpschen commented 2023-05-02 16:12:05 +00:00
Member

Mach mal in Crowdin ne Discussion auf, hier gehört das nicht hin :)

Discussion: https://crowdin.com/project/vikunja/discussions/9

> Mach mal in Crowdin ne Discussion auf, hier gehört das nicht hin :) Discussion: https://crowdin.com/project/vikunja/discussions/9
dpschen marked this conversation as resolved
src/views/tasks/TaskDetailView.vue Outdated
@ -322,6 +322,7 @@
@click="setFieldActive('priority')"
variant="secondary"
icon="exclamation"
v-shortcut="'p'"
dpschen commented 2023-04-20 14:41:22 +00:00
Member

Not important for this small PR, but a general reminder, that we talked about at some other places before:

These shortcuts really should be imported from a central place. That would also be the first step for possible user-customized shortcuts in the future.

Not important for this small PR, but a general reminder, that we talked about at some other places before: These shortcuts really should be imported from a central place. That would also be the first step for possible user-customized shortcuts in the future.
primeapple marked this conversation as resolved
dpschen added the
kind/feature
good first issue
 labels 2023-04-20 14:41:59 +00:00
konrad was assigned by dpschen 2023-04-20 14:42:09 +00:00
primeapple requested review from dpschen 2023-04-21 10:44:39 +00:00
primeapple force-pushed add-hotkeys-for-priority-and-delete from d0d1b88f6e to 920532f8fe 2023-04-21 18:43:35 +00:00 Compare
konrad reviewed 2023-04-24 10:58:01 +00:00
src/components/misc/keyboard-shortcuts/shortcuts.ts Outdated
@ -143,0 +151,4 @@
{
title: 'keyboardShortcuts.task.favorite',
keys: ['s'],
},
konrad commented 2023-04-24 10:58:01 +00:00
Owner

Please format this using tabs so that it's consistent with the rest of the document (why didn't the linter catch it?)

Please format this using tabs so that it's consistent with the rest of the document (why didn't the linter catch it?)
primeapple commented 2023-04-26 11:20:09 +00:00
Author
Contributor

The problem here is, that the .editorconfig actually wants 2 spaces indentation for .json files.

As for the reason why the Linter doesn't catch this:
The lint task in package.json only runs for .vue,.js,.ts files, not for json ones.

I wonder why you haven't set up prettier? Or even better rome (unsure if it works with vue files, tho).

~~The problem here is, that the `.editorconfig` actually wants 2 spaces indentation for `.json` files.~~ ~~As for the reason why the Linter doesn't catch this: The `lint` task in `package.json` only runs for `.vue,.js,.ts` files, not for `json` ones.~~ I wonder why you haven't set up prettier? Or even better [rome](https://rome.tools/) (unsure if it works with `vue` files, tho).
primeapple commented 2023-04-26 11:25:06 +00:00
Author
Contributor

Fixed it :)

Fixed it :)
konrad commented 2023-04-26 13:25:46 +00:00
Owner

We have a PR which got kind of stale: #930

Rome looks interesting though!

We have a PR which got kind of stale: https://kolaente.dev/vikunja/frontend/pulls/930 Rome looks interesting though!
👍 1
konrad marked this conversation as resolved
primeapple added 1 commit 2023-04-26 11:24:36 +00:00
continuous-integration/drone/pr Build is passing Details
9dfaabc0b3 Correct Formatting
konrad approved these changes 2023-04-26 13:26:49 +00:00
konrad left a comment
Owner

LGTM

LGTM
primeapple commented 2023-04-27 19:04:42 +00:00
Author
Contributor
Copy Link

Is there anything we are waiting for, before merging? ;-)

Is there anything we are waiting for, before merging? ;-)
primeapple added 1 commit 2023-05-09 19:04:57 +00:00
continuous-integration/drone/pr Build is failing Details
30b7d0f3ed Changed shortcut to delte from `x` to `Shift+Delete`
konrad added 1 commit 2023-05-10 08:20:54 +00:00
continuous-integration/drone/pr Build is failing Details
285bb037fa
Merge branch 'main' into feature/new-shortcuts
konrad added 1 commit 2023-05-10 08:48:33 +00:00
konrad merged commit e00c9bb1af into main 2023-05-10 09:14:08 +00:00
konrad deleted branch add-hotkeys-for-priority-and-delete 2023-05-10 09:14:08 +00:00
konrad commented 2023-05-10 09:14:16 +00:00
Owner
Copy Link

Thanks again!

Thanks again!
❤️ 1 🎉 1
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
dpschen
konrad
Labels
Clear labels   
area/internal-code

   
changes requested

   
confirmed

   
dependencies

   
duplicate

This issue or pull request already exists

  
good first issue

   
help wanted

Need some help

  
hosting

   
invalid

Something is wrong

  
kind/bug

Something is not working

  
kind/feature

New feature

  
question

More information is needed

  
wontfix

This won't be fixed

No Label
area/internal-code
changes requested
confirmed
dependencies
duplicate
good first issue
help wanted
hosting
invalid
kind/bug
kind/feature
question
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
konrad
4 Participants
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: vikunja/frontend#3400
No description provided.
Delete Branch "primeapple/frontend:add-hotkeys-for-priority-and-delete"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?