Conversation
|
@KevinVandy have added examples as well. What is the process now. |
|
I just have to have enough time to review |
|
@KevinVandy how does it look? can i help with something? |
|
I'm doing a deeper review and testing now. First pass was just getting all the tests to pass |
@tanstack/angular-hotkeys
@tanstack/hotkeys
@tanstack/hotkeys-devtools
@tanstack/preact-hotkeys
@tanstack/preact-hotkeys-devtools
@tanstack/react-hotkeys
@tanstack/react-hotkeys-devtools
@tanstack/solid-hotkeys
@tanstack/solid-hotkeys-devtools
@tanstack/svelte-hotkeys
@tanstack/vue-hotkeys
@tanstack/vue-hotkeys-devtools
commit: |
paoloricciuti
left a comment
paoloricciuti
left a comment
There was a problem hiding this comment.
Took a look at the code part and there are a few confusing things...if the code is working either there's probably some edge case missing or the code can be simplified a lot while continuing to work.
If you need any help feel free to ping me!
| * let ref = $state<HTMLDivElement | null>(null) | ||
| * let count = $state(0) | ||
| * | ||
| * createHotkey('Mod+S', () => { | ||
| * console.log('Mod+S pressed') | ||
| * count++ | ||
| * }, { target: ref }) | ||
| * </script> | ||
| * | ||
| * <div bind:this={ref}> | ||
| * Count: {count} | ||
| * </div> |
There was a problem hiding this comment.
Rather than doing this I would create an attachment (docs: https://svelte.dev/docs/svelte/@attach) that can retrieve a reference automatically from the template without using an extra variable.
There was a problem hiding this comment.
Probably best as its own function since global hotkeys are a common use case.
const increment = createHotkeyAttachment('Mod+S', () => {
console.log('Mod+S pressed')
count++
});<div {@attach increment}>
Count: {count}
</div>There was a problem hiding this comment.
Or maybe: Always return an attachment/an object that can create an attachment and make it possible to disable the global nature of the hotkey. I.e.
const increment = createHotkey('Mod+S', () => {
console.log('Mod+S pressed')
count++
}, { target: null });| let callbackRef = callback | ||
| let managerRef = manager | ||
|
|
||
| $effect(() => { | ||
| callbackRef = callback | ||
| managerRef = manager | ||
| }) |
There was a problem hiding this comment.
This feels weird to me: callback and manager are not stateful variables so this $effect will never re-trigger no?
| ? document | ||
| : null | ||
|
|
||
| // Skip if no valid target (SSR or ref still null) |
There was a problem hiding this comment.
$effect doesn't run during SSR so if you were architecting around SSR maybe something can be simplified?
| * </script> | ||
| * | ||
| * <div> | ||
| * <button on:click={recorder.startRecording}> |
There was a problem hiding this comment.
| * <button on:click={recorder.startRecording}> | |
| * <button onclick={recorder.startRecording}> |
| * {recorder.recordedHotkey && ( | ||
| * <div>Recording: {recorder.recordedHotkey}</div> | ||
| * )} |
There was a problem hiding this comment.
| * {recorder.recordedHotkey && ( | |
| * <div>Recording: {recorder.recordedHotkey}</div> | |
| * )} | |
| * {#if recorder.recordedHotkey} | |
| * <div>Recording: {recorder.recordedHotkey}</div> | |
| * {/if} |
| const mergedOptions = { | ||
| ...getDefaultHotkeysOptions().hotkeyRecorder, | ||
| ...options, | ||
| } as HotkeyRecorderOptions | ||
|
|
||
| const recorder = new HotkeyRecorder(mergedOptions) | ||
|
|
||
| // Sync options on every render (same pattern as createHotkey) | ||
| // This ensures callbacks always have access to latest values | ||
| $effect(() => { | ||
| recorder.setOptions(mergedOptions) | ||
| }) |
There was a problem hiding this comment.
mergedOptions is not a stateful variable so this effect will always run once...we should probably make mergedOptions a derived (but then you should pay attention because accepting an object as argument instead of a getter means the user will not be able to reassign the whole options reactively but just mutate them)...I would probably switch every option to be a a getter.
There was a problem hiding this comment.
If the options were not spread, the individual properties could already be made reactive via get prop() { return ... } at the call site.
For APIs like this it can also be convenient to have the function take either a callback returning an options object or an options object directly. That way you do not necessarily need many smaller functions or get properties to have something reactive.
Forcing each property to be a callback makes the calling code a bit noisy, but you could also potentially accept T | (() => T) on each, depending on how much overloading one is comfortable with, though this will clash on properties that are already callbacks...
| // Refs to capture current values for use in effect without adding dependencies | ||
| let callbackRef = callback | ||
| let optionsRef = mergedOptions | ||
|
|
||
| $effect(() => { | ||
| callbackRef = callback | ||
| optionsRef = mergedOptions | ||
| }) |
There was a problem hiding this comment.
Bit of syntax chaos here.
- Old store syntax on
$heldKeys. - What looks like broken reactivity on
isShiftHeld(neither.current, nor$) class:active={...}could be replaced with the newerclsxobject syntax:class={{ active: ... }}
| const mergedOptions = { | ||
| ...getDefaultHotkeysOptions().hotkeyRecorder, | ||
| ...options, | ||
| } as HotkeyRecorderOptions | ||
|
|
||
| const recorder = new HotkeyRecorder(mergedOptions) | ||
|
|
||
| // Sync options on every render (same pattern as createHotkey) | ||
| // This ensures callbacks always have access to latest values | ||
| $effect(() => { | ||
| recorder.setOptions(mergedOptions) | ||
| }) |
There was a problem hiding this comment.
If the options were not spread, the individual properties could already be made reactive via get prop() { return ... } at the call site.
For APIs like this it can also be convenient to have the function take either a callback returning an options object or an options object directly. That way you do not necessarily need many smaller functions or get properties to have something reactive.
Forcing each property to be a callback makes the calling code a bit noisy, but you could also potentially accept T | (() => T) on each, depending on how much overloading one is comfortable with, though this will clash on properties that are already callbacks...
| import { HotkeysProvider } from '@tanstack/svelte-hotkeys' | ||
| </script> | ||
| <HotkeysProvider |
There was a problem hiding this comment.
I would promote the use of setHotkeysContext over this. The component is essentially pointless unless you have multiple providers for different parts of the application at the same time.
Just call the function in the script, save one level of nesting.
4 participants
🎯 Changes
✅ Checklist
pnpm run test:pr.🚀 Release Impact