-
Notifications
You must be signed in to change notification settings - Fork 27
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
const onKeyboardNav = (e: KeyboardEvent) => { | ||
if (e.key === "L" && e.ctrlKey && !e.altKey) { | ||
if (e.key === "L" && (isApple ? e.metaKey : 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 ? "Option" : "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.
}; | ||
|
||
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.
This added line is not executed by any test.
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.
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 |
fixes: #757