feat: Keyboard Shortcuts System #2365
Conversation
Introduces a new grid view mode for the files page, allowing users to switch between list and grid views. Includes: - New FilesGrid component - View mode toggle buttons - Shared props between list and grid views - Updated rendering logic to support both view modes
…shortcut modal to context
- Added a new "General" shortcut label in the app localization. - Refactored shortcut handling in the FilesGrid and FilesList components to use refs for focused file management. - Improved keyboard shortcut handling by integrating the useShortcuts hook for better organization and functionality. - Adjusted the order of shortcut groups in the shortcut modal for improved user experience.
|
Hi @SgtPooki, I’ve made an attempt at addressing the issue and would appreciate your review when you have a moment. |
It might take me a while to get to review this one thoroughly. |
|
No worries at all, I understand. Thanks for the heads-up, and I’ll be around if you need anything clarified when you get to it! |
SgtPooki
changed the title
There was a problem hiding this comment.
Thanks again for this @george-hub331, this is great. Sorry it took me so long to only partially get through it 😅
I only got through 16/26 files but it's looking pretty useful. Some changes requested, and some comments could use some discussion.
| useShortcuts([{ | ||
| keys: ['Meta', 'c'], | ||
| label: t('addConnection'), | ||
| action: () => { | ||
| setIsOpen((prev) => !prev) | ||
| }, | ||
| group: t('app:shortcutModal.general') | ||
| }, { | ||
| keys: ['Shift', 'F'], | ||
| label: t('peers:filterPeers'), | ||
| action: () => { | ||
| const filterInput = document.getElementById('peers-filter') | ||
|
|
||
| if (filterInput) { | ||
| filterInput.focus() | ||
| } | ||
| }, | ||
| group: t('app:shortcutModal.general') | ||
| }]) |
There was a problem hiding this comment.
This component shows just how simple it is to use.. very nice work.
Some potential improvements here:
- use useCallback to ensure the actions are handled appropriately in the component's lifecycles.
- use a ref instead of document.getElementById to keep react in the loop..
maybe the API could be updated to accept a ref and automatically focus it?
is it too crazy to have the shortcuts system modular so we could useShortcuts and pass an ID so actions could be added in the exact component impacted by the shortcut?
that way, AddConnection would just useShortCuts to add the shortcut needed for it, instead of adding them ALL at the top level, any component could easily add shortcut support by specifying an ID. If that shortcut module is enabled currently, it would add to it.. (we may not even need to do this.. I still need to dive into the actual hook)
for components that doen't show until someone acts on it, they could add or remove the shortcut based on state only they need to be aware of, instead of propdrilling?
There was a problem hiding this comment.
The hook was designed to work on a page by page basis, so in most cases, we’re not adding shortcuts globally. That said, I do like the idea of making it more modular so components can register/unregister their own shortcuts without prop drilling, could be a nice addition actually.. working on it🤔
|
So that you know, I'm having some ts migration issues while adding the diagnostics screen in #2392. I am considering allowing I want all new code to be typechecked so we're not exacerbating the problem of untyped code, but without needing new PRs to enlarge unnecessarily.. |
2 participants
Fixes #2350
Summary
This PR adds a centralized keyboard shortcuts system using React Context, making it easier to manage and customize keyboard shortcuts throughout the application.
Changes
Implementation Details
New improvements
Short Demo
demo.mp4