-
-
Notifications
You must be signed in to change notification settings - Fork 11
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
feat(overlay): open/close overlay on keypress #196
feat(overlay): open/close overlay on keypress #196
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #196 +/- ##
==========================================
- Coverage 32.40% 32.22% -0.18%
==========================================
Files 38 38
Lines 1858 1868 +10
Branches 70 70
==========================================
Hits 602 602
- Misses 1256 1266 +10 ☔ View full report in Codecov by Sentry. |
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.
Awesome! Gave it a quick test and it seems to work well. I Have two requests though.
Also, please rebase your branch. I just merged in an ESC key fix and I wanna make sure this doesn't collide after the fix. Thank you!
packages/overlay/src/App.tsx
Outdated
} | ||
}; | ||
|
||
document.addEventListener('keydown', handleKeyPress); |
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.
Is there a specific reason you put this into the useEffect
block?
Asking because if we can move it to the top level of the App
component, I'd prefer using the useKeyPress
hook we already use for the ESC key navigation.
Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>
Hi @Lms24 based on your review above I checked useKeyPress hook I am thinking of something like this import { useEffect } from 'react';
/**
* useKeyPress
* @param {string[]} keys - an array of keys to respond to, compared against event.key
* @param {function} action - the action to perform on key press
* @param {boolean} propagate - whether to stop event propagation (default is false)
*/
export default function useKeyPress(keys: string[], action: () => void, propagate = false) {
useEffect(() => {
function onKeyup(e: KeyboardEvent) {
if (!propagate) e.stopPropagation();
if (keys.every((key: string) => {
if (key in e) return e[key as keyof KeyboardEvent];
return e.key.toLowerCase() === key.toLowerCase()
})) {
action();
}
}
window.addEventListener('keyup', onKeyup);
return () => window.removeEventListener('keyup', onKeyup);
}, [keys, action, propagate]);
} And to use this like this useKeyPress(['ctrlKey', 'shiftKey', 's'], () => {
setOpen(prev => !prev);
});
useKeyPress(['metaKey', 'shiftKey', 's'], () => {
setOpen(prev => !prev);
});
useKeyPress(['Escape'], () => {
if (location.pathname.split('/').length === 2) {
spotlightEventTarget.dispatchEvent(new CustomEvent('close'));
} else {
navigate(-1);
}
}); for single and multiple keys. Need review before pushing this. |
Sounds good to me! |
ok great! pushing it . |
… minor bug in Escape key handling hook to close when reached at last page
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.
💯
#158
Added an event listener for opening/closing overlay using
Ctrl + Shift + s
Also made this to astro dev as well.