caksoylar / keymap-drawer

Visualize keymaps that use advanced features like hold-taps and combos, with automatic parsing
https://caksoylar.github.io/keymap-drawer
MIT License
660 stars 57 forks source link

Assign key.type to combo CSS class #73

Closed englmaxi closed 7 months ago

englmaxi commented 7 months ago

Hi, while playing around with combos, I noticed that the type parameter assigned in raw_binding_map is not set to CSS classes of combos. I don't know if this is intentional (and this would have some downsides), but I would have assumed that the type is applied everywhere where the binding is used.

Example use case:

I use type to combine text and a glyph in some special cases:

parse_config:
  raw_binding_map: 
    '&mkp MB3':
      tap: $$mdi:cursor-default-click-outline$$
      hold: 3
      type: text_with_symbol

draw_config:
  svg_extra_style: |
    .text_with_symbol.tap {
      translate: -1px 1px;
    } 
    .text_with_symbol.hold {
      translate: 6px -31px;
      font-size: 8px;
    }   

    .combo.text_with_symbol.tap {
      translate: -1px 1px;
    } 
    .combo.text_with_symbol.hold {
      translate: 6px -13px;
      font-size: 8px;
    } 

At the moment, the type is not applied to combos: grafik

But with this PR, it would be: grafik


There is already a workaround by setting the style explicitly again for every combo:

parse_config:
  zmk_combos:
    combo_mb3:
      type: text_with_symbol

I find the proposed changes more intuitive and concise, especially if the same binding is used more than once. But please feel free to close this if you like it that way.


By the way, good job with this tool; it's fantastic!!

caksoylar commented 7 months ago

Hi there, thanks for the nice words. Your workaround with the CSS translations look super nice!

This was indeed intentional, see key (k) description in the combo spec. That being said, I don't have a strong opinion on it so I am OK changing this to work this way. If you can run black to fix the check and update the keymap spec to say that both (key and combo's own) types apply I can merge, or if you don't want to bother setting up the toolchain I can do that myself soon.

englmaxi commented 7 months ago

To be honest, I found the workaround at the end while writing the PR description. Since this is indeed intentional, I'd leave the decision up to you.

Either way, I have executed black and adapted the combo description.