-
-
Notifications
You must be signed in to change notification settings - Fork 80
Add key headers outside of hover state #157
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
Add key headers outside of hover state #157
Conversation
✅ Deploy Preview for zmk-studio ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
2a4be70 to
02ac8f4
Compare
|
Note - I think that maybe we can special-case Key Press or other behaviors that get special treatment. I think all of these things might improve the experience, but we have to start somewhere. I don't know what the community will land on, so I'm starting with something simple. |
|
I think ultimately having short names and/or icons in metadata is probably the best solution. Defining overrides now could mimic the same thing that the ultimate solution would bring - so I would be in favor of implementing overrides now with the eventual plan of relying on metadata instead of an override mapping in the future. In the meantime - I think I prefer the shortening vs the scaling. I worry with a too long behavior name, the scaling could make the text too small to read, especially on smaller screens. In your example, that looks good imo, but I think it also will limit the legible range of the headers. On the flip side - some keys do have more space for the normal size text. Not all keys are 1U. The scaling solution would handle this case elegantly. Another option would be to adjust the max length of a shortened header based on key size - but that also adds complexity. I'm leaning towards defining set overrides for now and using the existing shortening heuristic for anything without an override. We can define overrides for all of the standard ZMK behaviors. In the future, the metadata can provide the short header we should use, and it'd be up to module developers to provide reasonable short names. Thoughts? |
|
I think the lookup table for short names is the easiest to maintain and implement, especially if its only a short term solution. I dont think the short names will be too confusing for users, since they are probably already nerds. Do you have a way of getting all the standard behaviors? I personally wouldnt mind having keypress be listed as &kp |
src/keyboard/Key.tsx
Outdated
| > | ||
| <div className={`absolute text-xs ${selected ? "text-primary-content" : "z1text-base-content"} opacity-0 group-hover:opacity-80 top-1 text-nowrap left-1/2 font-light -translate-x-1/2 text-center transition-opacity`}>{header}</div> | ||
| <div className={`absolute text-xs ${selected ? "text-primary-content" : "z1text-base-content"} opacity-80 group-hover:opacity-0 top-1 text-nowrap left-1/2 font-light -translate-x-1/2 text-center transition-opacity`}>{shortenHeader(header)}</div> | ||
| <div className={`absolute text-xs ${selected ? "text-primary-content" : "z1text-base-content"} opacity-0 group-hover:opacity-80 top-1 text-nowrap left-1/2 font-light -translate-x-1/2 text-center transition-opacity`}>{shortenHeader(header)}</div> |
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.
One of these has to be redundant right? If we show the same text on the hovered keys, we dont need to fade out the small version, and fade in the big version
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.
Good catch! I fixed this.
I implemented the lookup table - plus some boilerplate for potential icons. I think having short names be as human friendly as possible would be ideal, and I took a stab at defining some short names along those lines. &kp makes sense if you've looked at the keymap files, but if you've only ever used Studio with ZMK / never looked at the code, it might not make as much sense. For all standard behaviors - I compiled some firmware without removing any behaviors and made short names for those. I'll go through the ZMK docs + code to see what else I need to add. We're missing a bunch (mouse emulation, backlight, soft off, underglow, sensor rotation) that I will try to add. |
|
Okay good job man, I think the shorthand names you selected are fine. I made an attempt at adding icons for the actual key values here #142. I dont think the solution is perfect, but I think it adds a lot visually using icons |
59c5e6e to
24279ab
Compare
petejohanson
left a comment
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.
src/keyboard/Key.tsx
Outdated
| if(typeof header === "undefined"){ | ||
| return ""; | ||
| } | ||
| const maxHeaderLength = 9; |
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.
I personally don't love constants hiding in functions, Can we pull this out so it's easier to find, and maybe capitalize, e.g. MAX_HEADER_LENGTH ?
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.
Good call! Done!
src/keyboard/Key.tsx
Outdated
| return ""; | ||
| } | ||
| const maxHeaderLength = 9; | ||
| if(shortNames[header]?.short != null){ |
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.
| if(shortNames[header]?.short != null){ | |
| if(shortNames[header]?.short){ |
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.
Empty string evaluates as falsy in JS - so I wanted to make this a null check, since we use empty string to define the "blank" override for "Key Press"
I get why this came up though, since it's not obvious at first glance, and is really for a special edge case just for Key Press.
Any suggestions as to best way to fix?
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.
Ah, fair. Ignore me then.
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.
Perhaps better to be !== which is a bit more explicit, and should work?
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.
Or at least a comment that this is a null check explicitly so empty strings are allowed as short names.
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.
yep - that should work. I think the != was a relic from when I didn't have an explicit undefined check above
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.
Ok, ignore me. The undefined check above is for the input header.
We need to use != since shortNames[header]?.short can be undefined.
I think a comment is most appropriate here. I got it wrong even though I implemented it initially
EDIT: I made this clearer by disallowing null in the typing. This makes the logic easier to understand, but I left a comment anyways
| "Grave/Escape": {"short": "Grv/Esc"}, | ||
| "Key Repeat": {"short": "Key Rept"}, | ||
| "Key Toggle": {"short": "Key Togg"}, | ||
| "Momentary Layer": {"short": "MomntLayr"}, |
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.
Not sure I love this one.... Maybe:
| "Momentary Layer": {"short": "MomntLayr"}, | |
| "Momentary Layer": {"short": "MLayer"}, |
| "Momentary Layer": {"short": "MomntLayr"}, | ||
| "Output Selection": {"short": "OutputSel"}, | ||
| "Sticky Key": {"short": "Stcky Key"}, | ||
| "Studio Unlock": {"short": "StdioUnlk"}, |
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.
| "Studio Unlock": {"short": "StdioUnlk"}, | |
| "Studio Unlock": {"short": "Unlock"}, |
| "Sticky Key": {"short": "Stcky Key"}, | ||
| "Studio Unlock": {"short": "Unlock"}, | ||
| "Toggle Layer": {"short": "Togg Layr"}, | ||
| "Transparent": {"short": "Trans"} |
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.
Sorry, last nit: Since we've got room, maybe this could be:
| "Transparent": {"short": "Trans"} | |
| "Transparent": {"short": "Transpnt"} |
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.
How about we drop all the vowels?
Trnsprnt
We also have room for
Transprnt
if we go by the 9 character max
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.
WFM.
petejohanson
left a comment
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.
LGTM, but can you please squash the commits into one proper feat commit, then we can merge?
096e67c to
4f74f40
Compare
Defines short names for behaviors for display on key headers
4f74f40 to
46acf08
Compare
|
Should be good to go now! |






Right now, ZMK Studio will show a blank key on keys that have any behavior set other than Key Press.
So at a glance, there isn't an easy way to differentiate between truly unbound keys, transparent keys, and keys with other behaviors assigned, such as studio unlock, bluetooth configuration, layer tap, mod tap, etc.
In order to see the behavior name, you have to hover over the key. This isn't ideal for a number of reasons.
This change adds key headers to keys at rest, without hover state.
There is one key challenge though - which is the space. If the header overflows the key, the UI will look like a mess. To handle this, I implemented a very simple heuristic (which should probably be discussed more).
If there are 10 characters or more, we try to split the behavior name into separate words (separated by a space or -) and then shorten each part equally to fit into 9 characters. This should handle most well formed behavior names, which are usually 2-3 words long.
If the human readable name of the behavior is 9 characters or less, we assume it'll fit on the key, and just display it as is.
Here's a few examples of what it looks like:


This meant to be up for discussion, this is just a starting point.