Skip to content
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

Assign key.type to combo CSS class #73

Merged
merged 3 commits into from
Feb 27, 2024

Conversation

englmaxi
Copy link
Contributor

@englmaxi englmaxi commented Feb 26, 2024

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
Copy link
Owner

caksoylar commented Feb 26, 2024

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
Copy link
Contributor Author

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.

@caksoylar caksoylar merged commit 6defcaf into caksoylar:main Feb 27, 2024
1 check passed
@englmaxi englmaxi deleted the combo-key-type branch February 27, 2024 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants