feat: keyboard shortcuts switcher in Settings#1684
feat: keyboard shortcuts switcher in Settings#1684alex-key wants to merge 6 commits intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
alex-key
changed the title
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
alex-key
changed the title
📝 WalkthroughWalkthroughThis PR adds a keyboardShortcuts setting and a new useKeyboardShortcuts() composable (default true), wires that state into multiple components, and gates existing single-key handlers behind it. Global and component-level key handlers (app.vue, AppHeader, package page, PackageSelector, etc.) now early-return when keyboardShortcuts is disabled. Keyboard hint UI and aria-keyshortcuts are rendered client-side and only when shortcuts are enabled. The settings page gains a toggle for keyboard shortcuts, translation/schema entries were added, and tests were updated to cover enabled/disabled behaviours. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
app/components/Link/Base.vue (1)
117-125: Consider reserving shortcut badge space to reduce hydration shift.
<ClientOnly>removes the<kbd>during SSR and inserts it on hydration, which can nudge neighbouring header controls. A fixed-size placeholder whenariaKeyshortcutsexists would keep layout stable.app/components/Button/Base.vue (1)
30-30: Consider extracting the composable call to the top level of setup.Invoking
useKeyboardShortcuts()inside a computed getter is unconventional. Composables are typically called at the top level of<script setup>to ensure proper reactive context and lifecycle handling.The current implementation works because
import.meta.clientguards the call on the server, but for clarity and adherence to Vue conventions, consider:♻️ Suggested refactor
+const keyboardShortcuts = import.meta.client ? useKeyboardShortcuts() : shallowRef(false) +const keyboardShortcutsEnabled = computed(() => keyboardShortcuts.value) -const keyboardShortcutsEnabled = computed(() => import.meta.client && useKeyboardShortcuts().value)test/e2e/interactions.spec.ts (1)
264-273: Consider adding a test for the 'c' shortcut when disabled.For completeness, you might want to add a test verifying that the 'c' shortcut (compare) also does not navigate when shortcuts are disabled. This would mirror the enabled test at lines 155-165.
🧪 Optional test to add
test('"c" (package) does not navigate to compare when shortcuts are disabled', async ({ page, goto, }) => { await goto('/package/vue', { waitUntil: 'hydration' }) await page.keyboard.press('c') await expect(page).toHaveURL(/\/package\/vue$/) })
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
app/app.vueapp/components/AppFooter.vueapp/components/AppHeader.vueapp/components/Button/Base.vueapp/components/Compare/PackageSelector.vueapp/components/Link/Base.vueapp/composables/useSettings.tsapp/pages/package/[[org]]/[name].vueapp/pages/settings.vuei18n/locales/en.jsoni18n/schema.jsonlunaria/files/en-GB.jsonlunaria/files/en-US.jsontest/e2e/interactions.spec.tstest/nuxt/composables/use-settings.spec.ts
alex-key
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/components/Link/Base.vue (1)
118-126: Consider reserving space for the client-only<kbd>hint to reduce layout shift.Because the key hint only appears after hydration, link/button content can shift horizontally. A small fallback placeholder in
ClientOnlywould minimise CLS in headers and dense layouts.♻️ Suggested tweak
- <ClientOnly> + <ClientOnly> <kbd v-if="keyboardShortcutsEnabled && ariaKeyshortcuts" class="ms-2 inline-flex items-center justify-center size-4 text-xs text-fg bg-bg-muted border border-border rounded no-underline" aria-hidden="true" > {{ ariaKeyshortcuts }} </kbd> + <template `#fallback`> + <span + v-if="ariaKeyshortcuts" + class="ms-2 inline-block size-4" + aria-hidden="true" + /> + </template> </ClientOnly>
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/components/AppFooter.vueapp/components/Compare/PackageSelector.vueapp/components/Link/Base.vuei18n/locales/en.jsoni18n/schema.jsonlunaria/files/en-GB.jsonlunaria/files/en-US.jsontest/nuxt/composables/use-settings.spec.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- i18n/locales/en.json
- test/nuxt/composables/use-settings.spec.ts
- app/components/Compare/PackageSelector.vue
- i18n/schema.json
- app/components/AppFooter.vue
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/components/AppFooter.vuetest/nuxt/composables/use-settings.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/nuxt/composables/use-settings.spec.ts
| <i18n-t keypath="shortcuts.disable_shortcuts" tag="span" scope="global"> | ||
| <template #settings> | ||
| <NuxtLink | ||
| :to="{ name: 'settings' }" |
There was a problem hiding this comment.
Link should target the keyboard-shortcuts section directly.
Line 103 currently routes to the settings page root only. To meet the “direct to setting” objective, point this link to the keyboard shortcuts section anchor instead of the page top.
Suggested change
- :to="{ name: 'settings' }"
+ :to="{ name: 'settings', hash: '#keyboard-shortcuts' }"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| :to="{ name: 'settings' }" | |
| :to="{ name: 'settings', hash: '#keyboard-shortcuts' }" |
1 participant
🔗 Linked issue
Resolves #1537
🧭 Context
In order to meet a11y requirements we should allow to disable custom keyboard shortcuts, which might conflict with other browser/OS shortcuts.
From Issue:
📚 Description
Basically checklist above describes changes. Added e2e to check for correct behavior on disabled shortcuts (last screenshot below).
Also see ss below for reference.
@knowler Since
<kbd>rendering became<ClientOnly>it might cause layout shift (buttons in header). Please comment if this is an issue.