feat: keyboard shortcuts switcher in Settings#1684
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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements a feature toggle mechanism to disable keyboard shortcuts throughout the application to meet WCAG 2.2 Success Criterion 2.1.4. A new 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' }" |
…/kbd-shortcuts-switcher
knowler
left a comment
knowler
left a comment
There was a problem hiding this comment.
Nice work. I do think we should address the layout shift issue. I’m not sure if there’s a way to access the setting early (e.g. in a blocking way in the <head>) and then use CSS show/hide the shortcuts? There could be other ways to address the issue (e.g. creating the layouts in a way that contents don’t cause other things to shift; ensuring heights aren’t affected by the presence of the shortcuts).
|
tada! no more layout shift 🚀 |
|
Thank you for the help, Daniel |
3 participants
🔗 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.