feat(minttea): supporting ctrl-key modifiers #45
Conversation
minttea/event.ml
Outdated
| @@ -1,5 +1,8 @@ | |||
| open Riot | |||
|
|
|||
| type modifier = Ctrl | |||
| type key_event = { key : string; modifier : modifier option } | |||
There was a problem hiding this comment.
suggestion. Could we move the modifier up to the KeyDown constructor?
type t =
| KeyDown of key * modifierI think this would allow you to pass in modifier for the special keys too, while keeping all the pattern matching ergonomic:
let update t e =
match e with
| KeyDown (Key "h" | Left, Ctrl) -> prev_page t
| KeyDown (Key "l" | Right, None) -> next_page tThere was a problem hiding this comment.
Didn't think of this, I think this is way better and more ergonomic, will add these changes
minttea/event.ml
Outdated
| @@ -1,5 +1,8 @@ | |||
| open Riot | |||
|
|
|||
| type modifier = Ctrl | |||
| type key_event = { key : string; modifier : modifier option } | |||
There was a problem hiding this comment.
question. Another thought is that to avoid the (Some Ctrl) wrapper when we want Ctrl we could make the modifier type just include a "no-modifier" variant like No_modifier:
type modifier = | No_modifier | CtrlStuffing this type with combinations would also make it easier to check for Ctrl + Shift by just using CtrlShift instead of trying to have a set / list.
What do you think?
There was a problem hiding this comment.
I think this is really good, I just added option as it made sense that no modifiers could just be None, but either way works fine. But I do enjoy not have Some/Nones. To me this suggestion is great
|
Hi @wllfaria, thanks for the PR! ✨ – left you some thought around the DX and how to keep extending the modifiers while allowing them on all possible keys (including the ones we special cased). Let me know what you think :) |
|
I've added updates addressing your considerations. Let me know if you think something should be different. |
|
@wllfaria changes look good to me! Thanks again for the PR 🙏🏼 There's one thing with some of the gifs (eg. the input field + cursor, list demo gif) where the behavior looks slightly different. I think it could just be an issue with the recording, but if you could have a look that'd be super appreciated. Both of those gifs show a "j" right on the last interaction that I don't think was there before, but I can't see that anywhere in the code. Any thoughts? Otherwise we can merge this as is and if you find the problem we can open an issue and fix that separately. |
|
That was a good catch from you, turned out that was a issue with how enter was getting parsed. Enter is sent as \n, and this matches against \x01-\x1a, I added a special case for \n before the matching and this should be fixed now. I think it would be good to maybe implement testing for this, so we can guarantee that further changes don't break past implementations, I can discuss this further here or in discord if you want. |
|
This looks great now! 🙌🏼 Could you make a new issue to discuss this testing? The discord is also good but it'd be nice to have a reference back to this. We can of course discuss first in discord and then make an issue too :) up to you! Again, thanks for the PR ✨ |
This patch aims to add support for Ctrl modifiers on keys.
This is a breaking change on the current interface of how key events are disposed to users.
NOTE: the amount of changes is due to re-creating all the examples and having to account for the change on the Key type.
I can either continue to add support for other modifiers here or in other Pull requests. Either way works for me.
Relates to #3