-
Notifications
You must be signed in to change notification settings - Fork 528
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
Implement UX Logging and Fix LLM Translation Shortcut Functionality #3241
Conversation
b93b5b3
to
bbd4c84
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a combination of 2-3 different sets of changes that would probably be clearer in separate PRs, but in general the direction for each of them seems fine.
@@ -59,7 +59,10 @@ export const LLMTranslationProvider: React.FC = ({ children }) => { | |||
stateRef.current.set(mt, { | |||
loading: false, | |||
selectedOption: characteristic, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit confusing that "selected option" and "characteristic" are names for essentially the same value, which is typed only as a string
. These values should be typed more strictly, with something like
type Characteristic = 'rephrased' | 'formal' | 'informal'
Also, the variable names should be consistent, so selectedCharacteristic
instead of selectedOption
.
export type SelState = { | ||
loading: boolean; | ||
selectedOption: string; | ||
llmTranslation: string; | ||
llmTranslations: Record<string, string>; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unclear why this refactor is included in a fix for keyboard shortcuts.
Fully fixes #3204
This PR includes the following changes:
Refactored the LLM hook: The LLM hook (
useLLMTranslation
) was refactored to return a function that is called outside of the callback. This change was made to address issues with theuseHandleCtrlShiftArrow
function, which previously caused the keyboard shortcut to stop functioning correctly after an LLM translation was generated.Added UX logging: Implemented UX logging for when LLM translations are copied into the editor using the keyboard shortcut. This helps track the adoption and usage of the LLM translation feature.
Issue Resolved
Note
As discussed with @eemeli, there is a need for further refactoring to fully address this issue. Specifically, issue #3305 - "When the main edit box is not in focus, the global arrow key combinations (ArrowDown or ArrowUp) do not function as expected" - should be addressed in the future. While the current changes enable the feature to function as expected, a more comprehensive solution is still needed to address the underlying issue in the existing code.