feat(behaviors): persistent None/Transparent quick tiles (#16 step 1)#27
Conversation
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ❌ Deployment failed View logs |
zmk-studio-tweaks | a402ea7 | Jun 05 2026, 03:38 PM |
numachang
left a comment
There was a problem hiding this comment.
Reviewed the diff and the surrounding files. Overall this is a clean, purely-additive slice — no RPC/firmware changes, capability-aware tile resolution, good comments, and tests that document intent. Happy to see it merge. The notes below are all optional follow-ups.
1. Logic/style duplication (minor)
The new selectBehavior (BehaviorBindingPicker.tsx:277-283) is identical to the inline onClick in renderBehaviorChip (BehaviorBindingPicker.tsx:207-213) — same id !== behaviorId guard plus the setBehaviorId/setParam1(0)/setParam2(0) reset. The chip's onClick could just call selectBehavior(b.id). The quick-tile className (BehaviorBindingPicker.tsx:302-306) is also a verbatim copy of the chip className (BehaviorBindingPicker.tsx:214-218); extracting a small "selectable chip" helper would keep the two from drifting apart on a future style tweak.
2. A11y: same accessible name in two radiogroups
None/Transparent now appear as role="radio" in both the new "Quick clear" radiogroup and the "Behavior" radiogroup, so when one is active both report aria-checked=true (this is why the test moved from findByRole to findAllByRole([0])). It works, but ARIA-wise one logical selection now spans two groups. Modelling the quick tiles as aria-pressed toggle buttons instead of radios would avoid the double announcement. Note this mirrors the existing Behavior chips' pattern (native <button role="radio"> without arrow-key roving), so it's consistency rather than a regression — best handled in a separate pass if at all.
3. Confirmation only — firmware without Key Press
promoteToKeyPress is gated on keyPressId !== undefined and falls back to the dimmed placeholder otherwise (BehaviorBindingPicker.tsx:267-275). Correct and safe for a None/Transparent-only firmware. No change needed.
Tests look on point (tile persistence + one-click Key Press promotion). Items 1–2 are take-it-or-leave-it.
…chips Address review on #27: the quick-clear tiles duplicated renderBehaviorChip's inline onClick reset and its className verbatim. Hoist `selectBehavior` and a `chipClass` helper and use them from both, so a future style or reset tweak can't drift between the two. No behavior change. Refs #16. Co-Authored-By: Claude <noreply@anthropic.com>
|
Thanks for the review! Addressed item 1 in ef6d849. 1. Duplication — done. Hoisted 2. A11y (radio in two groups) — deferring, per your "separate pass if at all." Beyond the consistency-with-existing-chips point, 3. Key-Press-less firmware — agreed, no change.
|
…p 1) Clearing a key no longer needs a round trip through the behavior tabs. A persistent strip of None / Transparent tiles sits above the behavior picker, highlighting the active one, and is resolved capability-aware from the connected firmware's behaviors by displayName (a tile only renders when that behavior exists). While a clear tile is active the parameter panel now shows a *live* keycode grid instead of the dimmed placeholder: picking a key promotes the binding straight back to Key Press in one click. Other 0-param behaviors keep the dimmed placeholder. This is step 1 of the key-picker rethink (docs/design/key-picker-rethink.md): the smallest, purely-additive slice that removes the biggest friction without touching the firmware/RPC path. Refs #16. Co-Authored-By: Claude <noreply@anthropic.com>
…chips Address review on #27: the quick-clear tiles duplicated renderBehaviorChip's inline onClick reset and its className verbatim. Hoist `selectBehavior` and a `chipClass` helper and use them from both, so a future style or reset tweak can't drift between the two. No behavior change. Refs #16. Co-Authored-By: Claude <noreply@anthropic.com>
Earlier this slice put None/Transparent in a strip above the whole behavior picker. The issue's Step 1 calls for the tiles to sit on *every key-grid tab*, so move them into the keycode grid itself: HidUsagePicker now takes an optional `clearTiles` config and renders the tiles in the grid header (visible on Basic, ISO/JIS, Numpad, ... and during search), threaded through BehaviorBindingPicker -> BehaviorParametersPicker -> ParameterValuePicker so they appear on every keycode grid the binding shows. The tiles are `aria-pressed` toggles (not radios): they switch the whole binding's behavior and reflect which clear behavior, if any, is active — this also avoids the same accessible name living in two radiogroups. Resolution stays capability-aware (a tile renders only if the firmware exposes that behavior), and the live-grid promote-to-Key-Press in one click is unchanged. Tests updated to drive the in-grid tiles (pinned apart from keymap keys via the aria-pressed `pressed` filter); Vitest 70 passed. Refs #16. Co-Authored-By: Claude <noreply@anthropic.com>
Reflect the implementation decision for #16 step 1: None/Transparent tiles live on every key-grid tab (in HidUsagePicker's grid header, threaded down from the binding picker), not in a strip above the behavior picker. Tiles are aria-pressed toggles rather than radios, which also resolves the deferred "same name in two radiogroups" a11y note. Refs #16. Co-Authored-By: Claude <noreply@anthropic.com>
d83c39d to
c528bcc
Compare
Address review: place None/Transparent as actual keycaps inside each tab's key layout (a blank cap = None, a ▽ cap = Transparent), styled identically to a 1U key — same border, fill and size — so they read as keys rather than faint chips that blend into the background. Appended to the end of the Basic / ISO·JIS / Numpad rows, the Function+Nav and flat-category lists, and the search results. Also drop the now-redundant None/Transparent chips from the behavior groups, since the keycaps replace them. Refs #16. Co-Authored-By: Claude <noreply@anthropic.com>
The clear keycaps were a fixed 1U size, so on the flat-list tabs (whose keys are the larger w-16 cells) they looked too small. Match the cap size to the keys of the tab they sit in: 1U-shape size on Basic / ISO·JIS / Numpad / Function+Nav, w-16 on the flat-category lists and search results. Also drop the clear keycaps from the Other tab — it's a combobox search, not a key grid, so a keycap has no place there. Refs #16. Co-Authored-By: Claude <noreply@anthropic.com>
numachang
left a comment
There was a problem hiding this comment.
Reviewed the diff: tsc --noEmit clean, vitest run src/App.edit.test.tsx green (5/5), and I traced the clearTiles threading end to end. Overall a clean, genuinely additive Step 1 that matches the design note in #26. Nice work on the shared selectBehavior/chipClass hoist, the capability-aware displayName resolution, the aria-pressed toggle (which also retires the "same name in two radiogroups" a11y note), and the added integration tests.
One design point I'd like to confirm, plus a few low-priority nits.
Design point worth confirming
From a behavior that doesn't render a keycode grid, there's no longer a one-click path to None/Transparent.
This PR drops the None/Transparent chips from the behavior groups (tieredBehaviors filters out clearIds), and the clear tiles only render inside a HID-usage grid. So when the current binding is:
- a layer behavior (
&mo/<→ParameterValuePickerrenders the layer radiogroup, no grid → no tiles), or - another 0-param behavior like Caps Word (the dimmed-placeholder branch in
BehaviorParametersPickeris not passedclearTiles),
…there is no None/Transparent anywhere on screen. Clearing the key now takes two clicks (Key Press chip → None tile) where the old chip row took one.
Net: the common Key Press → None path improves, but non-keycode starting states regress slightly. The design note accepts dropping the chips and says "Done when: from any keycode tab, one click sets &none" — which is true, but it doesn't cover the case where you're not on a keycode tab to begin with.
Is that the intended trade-off for Step 1? If we want to close the gap cheaply, passing clearTiles to the dimmed-placeholder HidUsagePicker would at least restore one-click clear from Caps Word & friends (layer behaviors would still need a separate affordance since they have no grid).
Nits (low priority)
- Search with zero matches hides the clear tiles. In the
No keys matchbranch,trailingClearKeycapsUsageisn't rendered, so you can't clear a key while a non-matching search is active. Rendering the tiles in that branch too would keep them reachable. - The None keycap is a fully blank cap (
CLEAR_TILE_GLYPHS.None = ""), discoverable only via tooltip/aria-label. Matches the "blank = None" intent, but a totally empty cap can read as a rendering gap among the keys. A faint glyph/label might help. - Firmware without a Key Press behavior (unrealistic):
keyPressId === undefined⇒ None/Transparent fall back to the dimmed placeholder (noclearTiles), so with the chips gone there's no way out. Not worth handling, just noting.
None of these block merge — approve-leaning once the design point above is confirmed.
🤖 Generated with Claude Code
Address PR review on #27: - None now shows a faint ∅ glyph instead of a fully blank cap (a blank cap read as a rendering gap among the keys). Both clear glyphs render faintly so the caps read as special "clear" keys. - Keep the clear tiles reachable when a search matches nothing (the "No keys match" branch rendered no tiles, so a key couldn't be cleared mid non-matching search). The reviewed design point — that dropping the chips removes the one-click clear from states with no keycode grid (layer behaviors, Caps Word) — is accepted as the Step 1 trade-off and documented in the design note; the slot model in Step 2 unifies clearing across all states. Refs #16. Co-Authored-By: Claude <noreply@anthropic.com>
|
Thanks for the thorough review! Addressed in da9921a. Design point — one-click clear from non-grid states: accepting this as the Step 1 trade-off, and documented it explicitly in the design note. The common Key Press ↔ None path improves; clearing from a state that renders no keycode grid (layer behaviors, Caps Word) takes two clicks for now. I deferred the cheap partial fix (passing Nit 1 — empty search hides the tiles: fixed. The clear tiles now render in the Nit 2 — fully blank None cap: fixed. None now shows a faint Nit 3 / point 3 — firmware without Key Press: acknowledged, leaving as-is (unrealistic;
|
Per follow-up: a glyph on the None cap reads as a specific key, which is misleading for "no binding". Leave it a blank keycap (its tooltip/aria-label still says "None") and drop the faint-glyph treatment, so None looks empty and Transparent keeps its ▽. Refs #16. Co-Authored-By: Claude <noreply@anthropic.com>
Step 1 of the key-picker rethink (#16, design note in #26). The smallest, purely-additive slice that removes the biggest friction.
What changed
displayName, so a tile only renders when that behavior exists.No firmware/RPC changes — output is still a
BehaviorBindingthrough the existingdoUpdateBindingpath.Tests
npm run lint— 0 errors (pre-existing warnings only)tsc --noEmit— cleanApp.edit.test.tsxcovering tile persistence and the one-click Key Press promotion.Refs #16. Builds toward the staged plan in
docs/design/key-picker-rethink.md(#26).🤖 Generated with Claude Code