add transformers api #20
Conversation
README.md
Outdated
| @@ -61,14 +61,15 @@ import { | |||
| hotkeyKeyUX, | |||
| jumpKeyUX, | |||
| focusGroupKeyUX, | |||
| overrides, | |||
There was a problem hiding this comment.
Let’s add second word to the name to avoid possible conflict with user code.
Like overrides → hotkeyOverrides
index.d.ts
Outdated
| * getHotKeyHint(window, 'b', [hintOverrides(config)]) // Alt + B | ||
| * ``` | ||
| */ | ||
| export function hintOverrides(config: HotkeyOverride): Transformer |
There was a problem hiding this comment.
It is very easy to user to send overrides instead of hintOverrides.
One solution is to add protection with types adding 2 different Transformer types.
Another option is to make transformers both-way. I like this idea because seems like we will need hotkey/hint versions of transformer for every transformer.
There was a problem hiding this comment.
Yes, let's go with both-way transformers.
index.d.ts
Outdated
| @@ -38,6 +38,10 @@ export interface FocusGroupKeyUXOptions { | |||
|
|
|||
| export type HotkeyOverride = Record<string, string> | |||
|
|
|||
| export interface Transformer { | |||
| (code: string, window: MinimalWindow, dir?: 'r'): false | string | |||
There was a problem hiding this comment.
Seems like dir: 'r' code is completely different from normal one.
Maybe we should not use the single function, but have 2?
export type Transformer = [
(code: string, window: MinimalWindow): false | string,
(code: string, window: MinimalWindow): false | string
]There was a problem hiding this comment.
For Mac compatibility transformer from #15 the code will be more similar at will benefit from having a single function. So it's a trade-off.
But aesthetically I prefer two functions.
There was a problem hiding this comment.
OK, let’s use argument. What do you think if we call it reverse: boolean?
There was a problem hiding this comment.
I tried both approaches, and in terms of compressed size two functions have a slight advantage. Since it's also aesthetically nicer, let's go that way? I updated the PR.
|
Looks great. Thanks. |
Continuing from this comment: #15 (comment)
Implementing the transformers API and switching overrides to transformers.