-
Notifications
You must be signed in to change notification settings - Fork 40
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
[BX-565] Keyboard shortcut and navigation tracking #837
Conversation
BX-565 Keyboard shortcut and navigation tracking
Shortcut event (when hotkeys or shortcuts are fired)
Navigation event (when user navigates with their keyboard, i.e. tab, arrow keys, or enter)
|
Here's the packed extension for this build: |
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.
Looking good 👌
const currentScreen = screen[pathname]; | ||
const trackNavigation = useCallback( | ||
({ key }: { key: string; type: KeyboardEventDescription }) => { | ||
analytics.track('keyboard.navigation.triggered', { |
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.
Would use event.keyboardNavigationTriggered
instead here
); | ||
const trackShortcut = useCallback( | ||
({ key, type }: { key: string; type: KeyboardEventDescription }) => { | ||
analytics.track('keyboard.shortcut.triggered', { |
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.
Would also use event.keyboardShortcutTriggered
here
// hackery preventing GweiInputMask from firing an onChange event when opening the menu with KB | ||
setTimeout(() => openCustomGasSheet(), 0); | ||
} else if (e.key === shortcuts.global.OPEN_GAS_MENU.key) { | ||
trackShortcut({ | ||
key: shortcuts.global.OPEN_GAS_MENU.display, | ||
type: 'gasMenu.open', |
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.
we don'y have / need consts for this?
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.
aah i see KeyboardEventDescription, is other thing?
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.
Yeah, KeyboardEventDescription
will show a type error if a random string is used and also provides autocompletion.
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.
lgtm
Here's the packed extension for this build: |
Here's the packed extension for this build: |
can't we have the |
Here's the packed extension for this build: |
Fixes BX-####
Figma link (if any):
What changed (plus any additional context for devs)
Screen recordings / screenshots
What to test
Final checklist
yarn build
).