-
Notifications
You must be signed in to change notification settings - Fork 32
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
Support Mac specific keybinds #762
base: main
Are you sure you want to change the base?
Conversation
I am a Mac friend. I see you want the keyboard commands tested? |
Hello @mac2net thanks for your interest to help me test this!
pull and checkout to this branch
and then you can follow README.md to build and install
Now you should have cockpit and cockpit-files ready so please connect to the webui using your macbook and try all of the keybinds listed in the help menu (the screenshot above). You might need to refresh the page if you had the webui running before installing cockpit-files. |
Hi @tomasmatus |
@mac2net yes, if you already have |
Hi @tomasmatus There are issues, mostly to do with visual feedback when navigating around. I compared it with the experience in XFCE accessed via VNC with a Mac app called Jump. There are some issues there too. I don't do a lot of keyboard stuff inside a web page on the Mac, but the MacOS Finder UI is ingrained in my DNA. If you don't have a Mac you should take a trip to an Apple Store to test this. While it is probably not possible or even desired to reproduce the behaviour of the Finder, it is the starting point for Mac users with Cockpit Files. There is one conflict with Safari – Command-Shift-L opens the left Tab Manager sidebar and in Firefox nothing happens, probably not allowed. Via VNC Control Shift L works fine |
}; | ||
|
||
const onKeyboardNav = (e: KeyboardEvent) => { | ||
const ctrlKey = isApple ? e.metaKey : e.ctrlKey; |
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.
Coverage complains as it checks both sides of the branch, so isApple
is never true so no 💯 code coverage.
@tomasmatus a while back I got an offer from Simon from Image builder to do some testing on a M1. So if this is still good I can ask him. |
I can't find that shortcut for Safari here. But according to this website it is showing the reading list. So that needs to be changed. |
Tested with: Firefox on macOSOption + UP: pass. Safari on macOS:Option + UP: pass. Note that Option + LEFT and Option + RIGHT do not work. However, Cmd + LEFT and Cmd + RIGHT do work since they're the default back/forward buttons for both browsers. Note that on Safari Cmd + Shift + L means "show sidebar" on Safari. |
Thanks for the feedback @supakeen!
Right, go back and go forward should be the same as browser keybinds I'll change it accordingly.
Then I need to come up with something new... Cmd + Opt + L or is that too crazy?
I assume you know what you're doing but just to be sure - did you select (single click) a file before using this keybind? |
Cmd + left - back in history, browser default Cmd + right - forward in history, browser default Cmd + up - go up a dir Cmd + down - go into selected dir Cmd + Opt + l - edit path
const onKeyboardNav = (e: KeyboardEvent) => { | ||
if (e.key === "L" && e.ctrlKey && !e.altKey) { | ||
if (isApple ? (e.key === "l" && e.metaKey && e.altKey) : (e.key === "L" && e.ctrlKey && !e.altKey)) { |
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 added line is not executed by any test.
const ctrlString = isApple ? "Command" : "Ctrl"; | ||
const altString = isApple ? "Command" : "Alt"; |
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.
These 2 added lines are not executed by any test.
<kbd className="key">Ctrl</kbd> + | ||
<kbd className="key">Shift</kbd> + | ||
<kbd className="key">{ctrlString}</kbd> + | ||
<kbd className="key">{isApple ? "Option" : "Shift"}</kbd> + |
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 added line is not executed by any test.
const ctrlKey = isApple ? e.metaKey : e.ctrlKey; | ||
const altKey = isApple ? e.metaKey : e.altKey; |
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.
These 2 added lines are not executed by any test.
Sounds fine to me; let's see if it works :)
Sounds fine to me; let's see if it works :)
I did! |
fixes: #757