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

feat: add v-shortcut directive for keyboard shortcuts #942

Merged
konrad merged 21 commits from feature/shortcut into main 2021-11-13 20:28:29 +00:00
Conversation 2 Commits 21 Files Changed 18 +251 -394
konrad commented 2021-11-03 18:25:46 +00:00
Owner
Copy Link

This PR moves all keyboard shortcuts to the github/hotkey library, adding a light v-directive wrapper around it. This allows us to add shortcuts like g k to open the kanban board or switch between list views.

This PR moves all keyboard shortcuts to the [github/hotkey](https://github.com/github/hotkey) library, adding a light v-directive wrapper around it. This allows us to add shortcuts like `g k` to open the kanban board or switch between list views.
konrad added 3 commits 2021-11-03 18:25:47 +00:00
continuous-integration/drone/pr Build is passing Details
c79a22661f
chore: remove old shortkey implementation
konrad added 1 commit 2021-11-03 18:43:29 +00:00
continuous-integration/drone/pr Build is passing Details
36c4027af6
feat: add new shortcuts to docs
konrad added 1 commit 2021-11-03 18:47:56 +00:00
continuous-integration/drone/pr Build is passing Details
8f18ce63f8
chore: add uninstall to shortcut directive
dpschen reviewed 2021-11-03 18:53:37 +00:00
src/i18n/lang/en.json Outdated
@ -758,0 +759,4 @@
},
"list": {
"title": "List Views",
"switchToListView": "Switch to list view",
dpschen commented 2021-11-03 18:53:37 +00:00
Member

Use these titles also as title on the links

Use these titles also as title on the links
konrad commented 2021-11-03 19:00:53 +00:00
Author
Owner

Done.

Done.
👍 1
konrad marked this conversation as resolved
dpschen reviewed 2021-11-03 18:54:23 +00:00
src/components/home/topNavigation.vue Outdated
@ -14,3 +14,2 @@
class="menu-show-button"
@shortkey="() => $store.commit('toggleMenu')"
v-shortkey="['ctrl', 'e']"
v-shortcut="'Control+e'"
dpschen commented 2021-11-03 18:54:23 +00:00
Member

Does this also support cmd on mac? That was something that always annoyed me a bit.
Speaking for all occasions of Control

Does this also support `cmd` on mac? That was something that always annoyed me a bit. Speaking for all occasions of `Control`
konrad commented 2021-11-03 18:55:57 +00:00
Author
Owner

I think that would be Meta. Will add it.

The full list is here btw: https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key/Key_Values

I think that would be `Meta`. Will add it. The full list is here btw: https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key/Key_Values
👍 1
dpschen commented 2021-11-03 19:02:57 +00:00
Member

I always use https://keycode.info/ =)

I always use https://keycode.info/ =)
konrad commented 2021-11-03 19:11:22 +00:00
Author
Owner

I've added a check to switch the shortcuts based on the user agent. Maybe a bit overengeneered, but makes for a great api.

Could you test it? I don't own a mac...

I've added a check to switch the shortcuts based on the user agent. Maybe a bit overengeneered, but makes for a great api. Could you test it? I don't own a mac...
dpschen commented 2021-11-03 19:23:59 +00:00
Member

Might be the way to go.
I think you need add iOS to the test aswell (some people use iPads as working device with keyboard attached).

Not sure which method makes most sense. Pick one and I'll test everything together:
https://stackoverflow.com/questions/9038625/detect-if-device-is-ios

Might be the way to go. I think you need add iOS to the test aswell (some people use iPads as working device with keyboard attached). Not sure which method makes most sense. Pick one and I'll test everything together: https://stackoverflow.com/questions/9038625/detect-if-device-is-ios
konrad commented 2021-11-03 19:41:07 +00:00
Author
Owner

Added.

Added.
dpschen commented 2021-11-04 11:26:44 +00:00
Member

I don't have a keyboard on iOS. Rest seeemed to work on macOS & iOS.
Maybe we should rename the function to isAppleDevice to reflect the change.

I don't have a keyboard on iOS. Rest seeemed to work on macOS & iOS. Maybe we should rename the function to `isAppleDevice` to reflect the change.
👍 1
konrad commented 2021-11-09 18:53:34 +00:00
Author
Owner

Renamed.

Renamed.
konrad marked this conversation as resolved
dpschen reviewed 2021-11-03 18:56:04 +00:00
src/components/misc/keyboard-shortcuts.vue Outdated
@ -27,0 +34,4 @@
</div>
<p>
<strong>{{ $t('keyboardShortcuts.list.switchToListView') }}</strong>
<shortcut :keys="['g', 'l']" :combination="$t('keyboardShortcuts.then')"/>
dpschen commented 2021-11-03 18:56:04 +00:00
Member

This seems a bit error prone to sync the global shortcuts manually.
How about we create some object to save the info that can be accessed from everywhere

  • here
  • where they are used
This seems a bit error prone to sync the global shortcuts manually. How about we create some object to save the info that can be accessed from everywhere - here - where they are used
konrad commented 2021-11-03 18:57:26 +00:00
Author
Owner

But wouldn't the only difference then be that the information about the shortcuts is saved in another file instead of the component directly?

But wouldn't the only difference then be that the information about the shortcuts is saved in another file instead of the component directly?
dpschen commented 2021-11-03 19:04:00 +00:00
Member

Not just that. There is then one central place where all the shortcuts are defined. This can be handy in the future e.g. if we want to show shortcuts also in the quick command bar.

Not just that. There is then one central place where all the shortcuts are defined. This can be handy in the future e.g. if we want to show shortcuts also in the quick command bar.
👍 1
konrad commented 2021-11-03 19:36:49 +00:00
Author
Owner

I think that makes sense. Added.

I think that makes sense. Added.
konrad marked this conversation as resolved
dpschen reviewed 2021-11-03 18:57:21 +00:00
src/components/misc/keyboard-shortcuts.vue Outdated
@ -65,1 +90,4 @@
components: {Shortcut},
computed: {
isTaskPage() {
return this.$route.name === 'task.detail' ||
dpschen commented 2021-11-03 18:57:21 +00:00
Member

use an array + includes

use an array + `includes`
konrad commented 2021-11-03 19:13:18 +00:00
Author
Owner

Done!

Done!
konrad marked this conversation as resolved
konrad added 1 commit 2021-11-03 19:00:49 +00:00
konrad added 1 commit 2021-11-03 19:10:27 +00:00
continuous-integration/drone/pr Build is passing Details
a90b863590
feat: use command on mac and ctrl on other platforms
konrad added 1 commit 2021-11-03 19:13:11 +00:00
continuous-integration/drone/pr Build is passing Details
a2c0846a03
chore: simplify is task detail page check
konrad added 1 commit 2021-11-03 19:36:37 +00:00
continuous-integration/drone/pr Build is failing Details
cd380601c9
feat: move shortcuts to js file
konrad added 1 commit 2021-11-03 19:40:54 +00:00
continuous-integration/drone/pr Build is failing Details
3ce3a3af65
feat: also detect ipads in is macos
konrad added 1 commit 2021-11-03 19:43:08 +00:00
continuous-integration/drone/pr Build is passing Details
e7114d24e9
fix: lint
konrad requested review from dpschen 2021-11-03 19:44:32 +00:00
dpschen reviewed 2021-11-03 20:17:30 +00:00
src/components/misc/keyboard-shortcuts/shortcuts.js Outdated
@ -0,0 +5,4 @@
shortcuts: [
{
title: 'keyboardShortcuts.toggleMenu',
keys: ['ctrl', 'e'],
dpschen commented 2021-11-03 20:17:30 +00:00
Member

Since the platform will never change on runtime:
How about putting the logic from isMac ? k.replace('ctrl', '⌘') : k in this file (doing it once at the top).

Since the platform will never change on runtime: How about putting the logic from `isMac ? k.replace('ctrl', '⌘') : k` in this file (doing it once at the top).
konrad commented 2021-11-03 20:57:48 +00:00
Author
Owner

Makes sense, changed.

Makes sense, changed.
konrad marked this conversation as resolved
dpschen reviewed 2021-11-03 20:18:35 +00:00
src/components/misc/keyboard-shortcuts/shortcuts.js
@ -0,0 +4,4 @@
available: () => null,
shortcuts: [
{
title: 'keyboardShortcuts.toggleMenu',
dpschen commented 2021-11-03 20:18:35 +00:00
Member

I once had problems with vue-i18n and dynamic keys in production build. Maybe not an issue anymore 🤷‍♂️

I once had problems with vue-i18n and dynamic keys in production build. Maybe not an issue anymore 🤷‍♂️
konrad commented 2021-11-03 20:59:34 +00:00
Author
Owner

Good hint. Let me check that.

Good hint. Let me check that.
konrad commented 2021-11-03 21:12:06 +00:00
Author
Owner

It looks like this is working fine.

It looks like this is working fine.
👍 1
konrad marked this conversation as resolved
konrad added 1 commit 2021-11-03 20:57:40 +00:00
continuous-integration/drone/pr Build is passing Details
ff52ceb92e
chore: only check for mac once
dpschen reviewed 2021-11-04 11:35:03 +00:00
src/components/misc/keyboard-shortcuts/shortcuts.js
@ -0,0 +55,4 @@
},
{
title: 'keyboardShortcuts.task.title',
available: (route) => [
dpschen commented 2021-11-04 11:35:03 +00:00
Member

fancy! =)

fancy! =)
😆 1
konrad marked this conversation as resolved
dpschen reviewed 2021-11-04 11:37:04 +00:00
src/components/misc/keyboard-shortcuts/index.vue Outdated
@ -0,0 +6,4 @@
<div class="message is-primary" v-if="s.available($route) === null">
<div class="message-body">
{{ $t('keyboardShortcuts.allPages') }}
dpschen commented 2021-11-04 11:37:04 +00:00
Member 
s.available($route) ? $t('keyboardShortcuts.currentPageOnly') : {{ $t('keyboardShortcuts.allPages') }}
```vue s.available($route) ? $t('keyboardShortcuts.currentPageOnly') : {{ $t('keyboardShortcuts.allPages') }} ```
konrad commented 2021-11-09 18:55:12 +00:00
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
dpschen reviewed 2021-11-04 11:40:00 +00:00
src/components/misc/keyboard-shortcuts/index.vue Outdated
@ -0,0 +15,4 @@
</div>
</div>
<p v-for="(sc, si) in s.shortcuts" :key="si">
dpschen commented 2021-11-04 11:40:00 +00:00
Member

Picky: I just realize this is the perfect usecase for: <dl>

Picky: I just realize this is the perfect usecase for: [`<dl>`](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/dl)
konrad commented 2021-11-09 18:58:19 +00:00
Author
Owner

Probably yeah :)

Added it, hope I understood it correctly.

Probably yeah :) Added it, hope I understood it correctly.
konrad marked this conversation as resolved
konrad added 1 commit 2021-11-09 18:53:29 +00:00
konrad added 1 commit 2021-11-09 18:55:13 +00:00
continuous-integration/drone/pr Build is passing Details
ea67048eb6
chore: simplify message for route available
konrad added 1 commit 2021-11-09 18:57:54 +00:00
continuous-integration/drone/pr Build is passing Details
521a5c69b4
feat: use <dl> for keyboard shortcuts
konrad added 1 commit 2021-11-09 22:56:28 +00:00
continuous-integration/drone/pr Build is passing Details
a5d6873aac
Merge branch 'main' into feature/shortcut
konrad commented 2021-11-09 22:57:19 +00:00
Author
Owner
Copy Link

@dpschen Could you give this another review?

@dpschen Could you give this another review?
dpschen was assigned by konrad 2021-11-09 22:57:32 +00:00
dpschen requested changes 2021-11-11 01:18:23 +00:00
src/components/misc/shortcut.vue Outdated
@ -1,10 +1,10 @@
<template>
<span class="shortcuts">
<dd class="shortcuts">
dpschen commented 2021-11-11 01:17:37 +00:00
Member

Making the main element of shortcut.vue a <dd> works in this context, but might not in different ones. I think we should add a prop for the tag with the default prop being a div.

<component :is="tag" ...
Making the main element of `shortcut.vue` a `<dd>` works in this context, but might not in different ones. I think we should add a prop for the tag with the default prop being a `div`. ```vue <component :is="tag" ... ```
konrad commented 2021-11-13 16:18:17 +00:00
Author
Owner

Done.

Done.
dpschen commented 2021-11-13 19:32:17 +00:00
Member

Ahh interesting. I somehow always thought that is is a reserved prop name for vue. But seems like <component> really is just a component with the prop is that can mount other components =)

Branch looks good!

Ahh interesting. I somehow always thought that `is` is a reserved prop name for vue. But seems like `<component>` really is just a component with the prop `is` that can mount other components =) Branch looks good!
konrad marked this conversation as resolved
konrad added 3 commits 2021-11-13 16:18:14 +00:00
995bb3bbe5
Merge branch 'main' into feature/shortcut 
# Conflicts:
#	src/components/home/topNavigation.vue
#	src/components/misc/keyboard-shortcuts.vue
continuous-integration/drone/pr Build is failing Details
d926c4b99d
feat: make shortcut tag configurable
dpschen approved these changes 2021-11-13 19:39:58 +00:00
konrad added 2 commits 2021-11-13 19:50:20 +00:00
continuous-integration/drone/pr Build is passing Details
f3f0541427
fix: lint
frederick commented 2021-11-13 19:53:56 +00:00
Member
Copy Link

Hi konrad!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://942-featureshortcut--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 konrad! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://942-featureshortcut--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.
konrad merged commit feea191ecf into main 2021-11-13 20:28:29 +00:00
konrad deleted branch feature/shortcut 2021-11-13 20:28:30 +00:00
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
dpschen
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
dpschen
3 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#942
No description provided.
Delete Branch "feature/shortcut"

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?