fix(Tasks): implement hotkeysEnabled state to control keyboard shortcuts #426
Conversation
- Add `enabled` parameter to useHotkeys hook with default true value - Pass hotkeysEnabled state to all useHotkeys calls in Tasks component - Add hotkeysEnabled check in manual keyboard handler (ArrowUp/Down/Enter) - Add data-testid to tasks table container for testing - Add tests for useHotkeys enabled parameter - Add tests for hotkeys enable/disable on mouse hover
|
Thank you for opening this PR! Before a maintainer takes a look, it would be really helpful if you could walk through your changes using GitHub's review tools. Please take a moment to:
More information on how to conduct a self review: This helps make the review process smoother and gives us a clearer understanding of your thought process. Once you've added your self-review, we'll continue from our side. Thank you! |
| ) : ( | ||
| <div | ||
| ref={tableRef} | ||
| data-testid="tasks-table-container" |
There was a problem hiding this comment.
added data-testid for testing hover behavior
There was a problem hiding this comment.
implement checking of hotkeysEnabled state before executing
There was a problem hiding this comment.
added tests for hotkeysEnabled behavior
There was a problem hiding this comment.
added tests for enabled
| export function useHotkeys( | ||
| keys: string[], | ||
| callback: () => void, | ||
| enabled: boolean = true |
There was a problem hiding this comment.
enabled with default true and when it false, hook will return early without adding any event listeners
ShivaGupta-14
left a comment
ShivaGupta-14
left a comment
There was a problem hiding this comment.
self review done, ready for review
|
@ShivaGupta-14 this looks good but the mouse control check not work in scenarios though. Like for example a tablet with some keyboard. Probably need a better check, instead of this. |
|
thanks @its-me-abhishek! you are right it needs a better check. I researched this and I am planning to use the Active Context approach combined with the current implementation, so it will work on both touch/keyboard devices and with mouse hover. will push the update soon! |
2 participants
Description
Add
enabledparameter to useHotkeys hook with default true valuePass hotkeysEnabled state to all useHotkeys calls in Tasks component
Add hotkeysEnabled check in manual keyboard handler (ArrowUp/Down/Enter)
Add data-testid to tasks table container for testing
Add tests for useHotkeys enabled parameter
Add tests for hotkeys enable/disable on mouse hover
Fixes: bug: hotkeysEnabled state is set but never checked before executing hotkeys #425
Checklist
npx prettier --write .(for formatting)gofmt -w .(for Go backend)npm test(for JS/TS testing)Additional Notes
Video:
Screen.Recording.2026-01-22.at.1.17.29.AM.mov