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

feat: basic keyboard shortcuts #319

Merged
merged 38 commits into from
Mar 7, 2023
Merged

feat: basic keyboard shortcuts #319

merged 38 commits into from
Mar 7, 2023

Conversation

hartmut-co-uk
Copy link
Collaborator

@hartmut-co-uk hartmut-co-uk commented Dec 3, 2022

implements #39

TODOs

Options for improvement

  • performance / ressource utilization
    • disable event listeners for mobile devices (without hardware keyboard)
  • UI/UX
    • ??? for page navigation (e.g. g .. h / g .. n) show a notification message ~"Going to home" by @edimitchel
  • code / architecture
    • simplify / find better solution for j/k navigation.
      Current solution uses DOM & querySelectors, hard to maintain / comes with high risk of regressions while working on other features..
    • export composables for each keybinding, components self-register triggers, centralised tracking of listeners (ref discord) by @QiroNT

@stackblitz
Copy link

stackblitz bot commented Dec 3, 2022

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@netlify
Copy link

netlify bot commented Dec 3, 2022

Deploy Preview for elk-zone ready!

Name Link
🔨 Latest commit 48b729c
🔍 Latest deploy log https://app.netlify.com/sites/elk-zone/deploys/6406290890210400089b81b0
😎 Deploy Preview https://deploy-preview-319--elk-zone.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@hartmut-co-uk hartmut-co-uk self-assigned this Dec 4, 2022
@hartmut-co-uk
Copy link
Collaborator Author

Hmm the 'Compose' shortcut is triggered also for any combinations incl. c key - e.g. Shift+cmd+c.
I think it would be better to watch for single key's to be pressed exclusively?

@hartmut-co-uk
Copy link
Collaborator Author

OK, pushed some improvements, j/k nav on status detail page should now also work fine + highlight (outline) the top StatusDetail onfocus (press k)

@hartmut-co-uk
Copy link
Collaborator Author

f / b shortcuts also now work on default for 'status detail' page.. applied for the main status

@hartmut-co-uk
Copy link
Collaborator Author

I have tried to address issues with key 'exclusiveness' (example c) through manually testing that none of the keys [shift, cmd, alt, meta] are pressed in combination.

Also noticed a but with vueuse (vueuse/vueuse#2515).
Once that's merged and pulled in, I think it might be best to simply test for current.size === 1. Any thoughts?

(maybe exclusive feature could be added to vueuse.useMagicKeys?)

# Conflicts:
#	components/status/StatusCard.vue
@Shinigami92 Shinigami92 added the c: feature Request for new feature label Dec 6, 2022
@hartmut-co-uk hartmut-co-uk marked this pull request as ready for review December 6, 2022 23:42
@danielroe
Copy link
Member

This is beautiful. A couple of comments:

  1. after escaping from a dialogue, I then appear not to be able to tab any more. To reproduce, type c then escape and then try to use a keyboard shortcut
  2. I can't use j/k to go down notifications - sometimes a notification 'stops' the progress
  3. if we have (R)eply (B)oost (F)avourite and (B)ookmark maybe we should consider another hotkey for bookmark - (S)ave to Bookmarks?

@hartmut-co-uk
Copy link
Collaborator Author

related Akryum/vue-virtual-scroller#499
(though - we don't set focus on the virtual scroller wrapper divs)

The actual reason why current approach is not working with vue-virtual-scroller - is that the order of the items in the DOM parent element is totally different from what's displayed, since it's controlled by the virtual scroller via style (transform: translateY(2704px)):

image

So we should probably work with the data stream -> 'issues' array to track prev/active/next status, using status ids.

🤔 maybe active status can be added to usePaginator composable and key events registered there and passed down, I'll give that a try. It might become tricky though on status detail page, where the top-most status is not part of the stream...

# Conflicts:
#	components/modal/ModalContainer.vue
#	composables/dialog.ts
@hartmut-co-uk hartmut-co-uk marked this pull request as draft December 12, 2022 19:29
@KaKi87 KaKi87 closed this Jan 4, 2023
@KaKi87 KaKi87 deleted the feat/39-keyboard-shortcuts branch January 4, 2023 17:49
@danielroe danielroe restored the feat/39-keyboard-shortcuts branch January 4, 2023 17:57
@danielroe danielroe reopened this Jan 4, 2023
@jan-vandenberg
Copy link
Contributor

The shortcut on Twitter for bookmark is 'b' (this will either add or remove a post from the bookmarks).
image

@MForster
Copy link

(I hope you don't mind drive-by commens. Feel free to ignore)

This is great!

One way it could be improved: Currently the scrolling with j / k is somewhat unpredictable. If the newly selected status still fits on the page only the focus is moved. If it doesn't fit, the page scrolls, and the focused status moves to the center of the page. This makes it hard to follow the focus with my eyes, because it's hard to predict where the next status will be drawn on the page after navigation.

IMHO there would be two alternative approaches that each would work better:

  • Always scroll the newly focused status to the center, even if it isn't necessary
  • Only scroll as far as needed that the newly focused status fits on the page "just so".

Both options would make it easier to follow the focus.

Just my 2¢...

@netlify
Copy link

netlify bot commented Mar 5, 2023

Deploy Preview for elk-docs canceled.

Name Link
🔨 Latest commit 48b729c
🔍 Latest deploy log https://app.netlify.com/sites/elk-docs/deploys/64062908c59a0300089972ee

@hartmut-co-uk hartmut-co-uk marked this pull request as ready for review March 5, 2023 15:30
@hartmut-co-uk
Copy link
Collaborator Author

I've added z key shortcut for 'zen mode', but I removed j/k for prev/next status navigation to do in a follow-up task.

Follow-up tasks with proposed mappings

Therefore with the basics added and a first setup to build on - I think this is ready to be reviewed.
I left TODOs in the code and would love to get feedback and/or suggestions.
'Favourite' and 'Boost' is not solved properly but working. Maybe it would be better to solve this properly before merging this PR?

Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

Thanks a lot for taking the time to work on this feature and split the PR into mergeable pieces @hartmut-co-uk 🧡
It works great for the current set of shortcuts. I think we should move forward and keep iterating with your plan.

@patak-dev patak-dev changed the title feat: keyboard shortcuts feat: basic keyboard shortcuts Mar 7, 2023
@patak-dev patak-dev merged commit c4d8137 into main Mar 7, 2023
@patak-dev patak-dev deleted the feat/39-keyboard-shortcuts branch March 7, 2023 19:32
DataDrivenMD pushed a commit to Distal-Labs/elk that referenced this pull request Mar 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: feature Request for new feature
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

8 participants