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

fix(docs): Update tap-dance and hold-tap documentation #1298

Merged
merged 14 commits into from Jul 30, 2022

Conversation

kurtis-lew
Copy link
Contributor

@kurtis-lew kurtis-lew commented May 13, 2022

General

  • Replace raster timing diagrams with vector assets

Tap-Dance

  • Improve conciseness of tap-dance docs by adding tabs for example implementation
  • Expand upon tap-dance bindings and the effect of the binding index on the number of keypresses required to output the binding
    • Enhance bindings section further, explaining that the number of bindings is equal to the maximum number of keypresses

Hold-Tap

  • Convert postitional-hold-tap section to paragraph form
  • Condense homerow mods, Autoshift, Sticky Holds, and toggle/momentary layer examples into tabbed menu

@kurtis-lew
Copy link
Contributor Author

Addresses #298.

@kurtis-lew kurtis-lew force-pushed the fix(docs)-holdtap-tapdance branch 2 times, most recently from 5a1989f to 9572d0e Compare May 14, 2022 03:04
Copy link
Contributor

@caksoylar caksoylar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should make the figures more legible and make future modifications much easier as well. The advanced tap-dance example is also useful. Thanks!

docs/docs/behaviors/tap-dance.md Outdated Show resolved Hide resolved
@okke-formsma
Copy link
Collaborator

okke-formsma commented May 20, 2022

Some notes;

  1. hold-tap behavior, first image. The tap decision is actually made as soon as the HT key is released, not when the tapping term expires. I think it would make sense to keep the 'tapping_term' annotation everywhere the arrow appears so its clear what it represents. I don't see a clear reason why the line below the tapping_term is sometimes black and sometimes red.

image

  1. In the second image, do not make the key-up and tapping-term expiration occur at the same time, it suggests a relationship between these two values which is not present.

image

  1. In the big image, hold-preferred 4a 4b 4c, I would not show the tapping-term arrow because the tapping-term does not matter at all in this scenario.

image

  1. In the big image, hold-preferred 4d, do not show the tapping-term arrow and especially do not coincide the key-up with it's end, as it tapping-term does not have any influence on the behavior.

image

  1. Balanced, 4b: The text is wrong (this has been wrong for a long time). Should be "the behavior is decided as the other key is released".

image

  1. Balanced, 4c: The text is wrong. Should be "the behavior is decided as the other key is released".

image

  1. Balanced, 4d: Not sure about the tapping-term line being red again. Try to make the release of the F not coincide with the end of the tapping term in the image, as these are not related.

image

  1. Tap-preferred 4c, 4d: The decision is made as soon as the hold-tap key is released, not after the tapping term expires.

image

image

@kurtis-lew kurtis-lew force-pushed the fix(docs)-holdtap-tapdance branch 13 times, most recently from 0b3fb3b to 0195f5a Compare May 28, 2022 18:08
@kurtis-lew kurtis-lew force-pushed the fix(docs)-holdtap-tapdance branch 4 times, most recently from 8ab049e to 9db8a1c Compare June 4, 2022 03:23

<TabItem value="advanced">

This example configures a mod-tap inside a tap-dance named `td_mt` that outputs `LSHIFT` on a single tap, `CAPSLOCK` on a single press and hold, and `LCTRL` on double-tap.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that hold-tap (&ht) should be used here rather than mod-tap (&mt) because CAPSLOCK is not a modifier.

But I also think this example is a little confusing, because:

  1. SHIFT is normally held, but here it is output on a tap
  2. CAPSLOCK is normally tapped to toggle on/off, but here it is output on a hold
  3. CONTROL is also usually held, but here it is output on a double-tap

Apologies if I'm just misunderstanding, but I think the example might make more sense like so:

Suggested change
This example configures a mod-tap inside a tap-dance named `td_mt` that outputs `LSHIFT` on a single tap, `CAPSLOCK` on a single press and hold, and `LCTRL` on double-tap.
This example configures a hold-tap inside a tap-dance named `td_ht` that outputs `CAPSWORD` on a single tap, `SHIFT` on a single press and hold, and `CAPSLOCK` on double-tap.

This way, the tappy things are output with taps and the holdy things are output with holds.

Copy link
Contributor Author

@kurtis-lew kurtis-lew Jun 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of initial reactions:

  • CONTROL still makes sense on the double-tap because the tap-dance doesn't release the LCTRL until its own binding is released
  • &ht is not a pre-defined ZMK behavior; we'd have to create a separate hold-tap behavior that implements the &caps_word on single-tap and &kp LSHIFT on single-press-and-hold
    • Using &mt in the tap-dance achieves what most people are looking for in a tap-hold-dance, i.e., a different &kp for single-tap, single-press-and-hold, double-tap, double-press-and-hold, etc.

Otherwise, I agree with updating the example to ensure things that are usually held are output on holds. Perhaps we can update the existing example with the changes you've suggested and add another example that explicitly uses the &mt inside a tap-dance.

Update: Expanding on the third point, would it be worth mentioning in the mod-tap docs that the "hold" keycode doesn't necessarily need to be a modifier key?

Copy link
Contributor Author

@kurtis-lew kurtis-lew Jun 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like this for the updated version of the current example, perhaps:

Suggested change
This example configures a mod-tap inside a tap-dance named `td_mt` that outputs `LSHIFT` on a single tap, `CAPSLOCK` on a single press and hold, and `LCTRL` on double-tap.
This example configures a hold-tap inside a tap-dance named `td_ht` that outputs `&caps_word` on a single tap, `&kp LSHIFT` on a single press and hold, and `&kp CAPSLOCK` on double-tap.
```title="Advanced Tap-Dance Example: Nested Hold-Tap"
#include <behaviors.dtsi>
#include <dt-bindings/zmk/keys.h>
/ {
behaviors {
ht: hold_tap{
compatible = "zmk,behavior-hold-tap";
label = "HOLD_TAP";
#binding-cells = <2>;
tapping-term-ms = <200>;
flavor = "tap-preferred";
bindings = <&kp>, <&caps_word>;
};
td_ht: tap_dance_hold_tap {
compatible = "zmk,behavior-tap-dance";
label = "TAP_DANCE_HOLD_TAP";
#binding-cells = <0>;
tapping-term-ms = <200>;
bindings = <&ht LSHIFT 0>, <&kp CAPSLOCK>;
};
};
keymap {
compatible = "zmk,keymap";
default_layer {
bindings = <
&td_ht
>;
};
};
};

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh very good points!

I think a custom hold tap behavior overcomplicates things here. Also, because &caps_word doesn't take any parameters that makes for an awkward hold-tap example (i.e. &ht LSHIFT 0). I'm sorry for making such a poor suggestion. 😅

I believe there is one issue with the original example though – LSHIFT is unusable as it cannot be held? If so, I think all we really need to do is swap LSHIFT and CAPSLOCK in the example?

Suggested change
This example configures a mod-tap inside a tap-dance named `td_mt` that outputs `LSHIFT` on a single tap, `CAPSLOCK` on a single press and hold, and `LCTRL` on double-tap.
This example configures a mod-tap inside a tap-dance named `td_mt` that outputs `CAPSLOCK` on a single tap, `LSHIFT` on a single press and hold, and `LCTRL` on double-tap.

Copy link
Collaborator

@dxmh dxmh Jun 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expanding on the third point, would it be worth mentioning in the mod-tap docs that the "hold" keycode doesn't necessarily need to be a modifier key?

Yep, I had never checked the code and always assumed from our documentation that it needed to be a modifier. I think a small tip mentioning this would be a nice touch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good eye, but not to worry; on local testing, the link to keycode works fine since it's a relative path. As for mentioning that mod-taps can't take other types of bindings, it's another valid suggestion. I'll take a stab at writing a draft up right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we go the more comprehensive route, a note somewhere about the purpose of the dummy 0 in the &ht LSHIFT 0 would probably help too.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking we expand this tip section I'm working on to mention that the dummy value technique applies to all nested, zero-binding-cell behaviors too. For now, I don't have the brainpower to get to it at the moment, but I can take another shot at it tomorrow.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the docs need a bit more indication somewhere, or again for the sake of redundancy, wherever it's relevant, that behaviors can be combined.

@kurtis-lew I agree. I think maybe we could explore this goal separately though? Maybe we need a separate GitHub issue dedicated to documentation about explaining behaviors and using them in more advanced ways. (Personally I think it would be good to keep the pages for individual behaviours focused on their own functionality and usage.)

The blurb you've got in the PR currently is great, and I think for now it would be good not to expand the scope of this PR too much, so that we can merge the other good work that's already done.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I think I'll just come back to the code snippets posted here to copy-paste into another PR related to composite behaviors if need be. I think if we get the go-ahead from @okke-formsma, we should be good to merge!

@kurtis-lew kurtis-lew force-pushed the fix(docs)-holdtap-tapdance branch 2 times, most recently from 74e7589 to 306c109 Compare June 7, 2022 03:36
Copy link
Contributor

@caksoylar caksoylar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More nits and picks on the latest commit, thanks again!

docs/docs/behaviors/hold-tap.md Outdated Show resolved Hide resolved
docs/docs/behaviors/layers.md Outdated Show resolved Hide resolved
docs/docs/behaviors/layers.md Outdated Show resolved Hide resolved
docs/docs/behaviors/mod-tap.md Outdated Show resolved Hide resolved
docs/docs/behaviors/mod-tap.md Outdated Show resolved Hide resolved
@kurtis-lew kurtis-lew force-pushed the fix(docs)-holdtap-tapdance branch 3 times, most recently from 4259411 to 9938b0d Compare June 7, 2022 08:48
@dxmh
Copy link
Collaborator

dxmh commented Jun 11, 2022

Hey @okke-formsma, you left some comments in #1298 (comment).

Are you happy with how things are now? If so, I believe we're good to merge. 👍

Here's a link to the latest version: https://deploy-preview-1298--zmk.netlify.app/docs/behaviors/hold-tap

@kurtis-lew
Copy link
Contributor Author

@dxmh, at least from my end in the tap-dance department, I'm not sure if I'm ready for this to be merged. Could you look over this part again: #1298 (comment) ? Just want confirmation if we should keep the tap-dance docs as is with the simpler mod-tap inside of a tap-dance example, or if we should go the more detailed route.

The updated blurb I've linked above also contains the tip admonition that explains the purpose of dummy-zeroes in certain behaviors, as shown here #1298 (comment).

Copy link
Contributor

@caksoylar caksoylar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also prefer if we merge this then create a new PR for any improvements later on

@caksoylar caksoylar added the documentation Improvements or additions to documentation label Jul 4, 2022
kurtis-lew and others added 14 commits July 20, 2022 20:24
- Replace raster timing diagrams with vector assets
- Improve conciseness of tap-dance docs by adding tabs for example implementation
- Expand upon tap-dance bindings and the effect of binding index on the number of keypresses required to output the bindng

Co-Authored-By: Cem Aksoylar <caksoylar@users.noreply.github.com>
- Convert positional-hold-tap section to paragraph-form
- Various edits for flow and grammar
- Increase font size of output descriptors in `comparison.svg` from 30pt to 36pt
- Add **Common Use-Cases** section with tabs for homerow mods, autoshift, sticky-holds, and toggle/momentary holds
Co-Authored-By: Cem Aksoylar <caksoylar@users.noreply.github.com>
@jimmerricks has noted an issue where sticky keys get stuck for their full timer duration when typing quickly. While few users have reported a similar experience, we will still remove this hold-tap example until changes to the sticky key behavior are made to better facilitate this requested feature.
@dxmh dxmh self-requested a review July 26, 2022 10:03
Copy link
Collaborator

@dxmh dxmh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @kurtis-lew and everyone else involved in updating these docs. 🙂

This looks good for merge. 👍

@dxmh dxmh merged commit eee7e1c into zmkfirmware:main Jul 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants