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

[wip] Add new keyboard shortcuts #355

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

1Computer1
Copy link
Contributor

@1Computer1 1Computer1 commented Dec 13, 2021

Fixes #218:

  • Pause pauses in play, pause/enter/escape/space resumes in overlay.
  • !, @, # will check square, entry, grid, use with Ctrl to reveal.
  • @ toggles autofill in constructor.

Labeled WIP because I'm not entirely happy with this.

  • Need to move things inside reducer at this point, how to run constructor stuff and dispatch other events in the reducer? Or maybe don't have things inside reducer?
  • Not sure if those are good key bindings.
  • Probably want to make Key = { k: KeyK, mods: Modifier[] } or something.
  • Icons are pretty ugly lol, also maybe could be generalized?
  • How does shift-arrow navigation work? (Shift-arrow navigation #148) 🤔
  • Should escape just work on every possible menu?

@1Computer1 1Computer1 marked this pull request as draft December 13, 2021 00:49
@mdirolf
Copy link
Member

mdirolf commented Dec 13, 2021

I did some generalization of the icon code and replaced the ctrl-X style icons with a seperate ctrl + X icon, which I think looks a little nicer.

@mdirolf
Copy link
Member

mdirolf commented Dec 13, 2021

My primary concern with this is figuring out the right keystrokes to use. I'm hesitant to start using !, @ and # because I could imagine a (remote) chance that we might want to allow them in grids at some point. The problem with using a bunch of ctrl-X or alt-X keystrokes is that every browser/OS uses different ones and most use a bunch of them. And we're only going to want to add more keystrokes over time. Here's a suggested solution:

  • Do some research to figure out one cross-browser/os "safe" keystroke to use. Maybe it's a modified version like ctrl-i or maybe (i'm leaning towards this I think) it's a plain key that we're ok with permanently disallowing from grids, like /.
  • When that key is pressed change into a new hotkey mode in the reducer - until the hotkey is finished any future keystrokes are part of a chorded shortcut key. So for example / c g might check the grid, while / r c would reveal the current cell.
  • Hotkey mode would exit when a hotkey is finished or any key is pressed that isn't part of a hotkey (we should include escape in that list I think)
  • When in hotkey mode a big overlaid list of all shortcuts should appear, like what you see when you hit ? in gmail

This is obviously a little bit more complicated than the existing work, but I don't think it dramatically complicates things since we definitely want to move this stuff into the reducer anyway.

@mdirolf
Copy link
Member

mdirolf commented Dec 13, 2021

To answer your other questions:

  • We definitely should move this logic into the reducer - most of it is pretty straight forward. For example, instead of dispatching a check cell action when ! is pressed, we dispatch a regular keystroke action and add a case statement in the reducer for the ! keystroke that just does exactly what the check cell action does currently. I think the only thing that requires more changes is disabling autofill because it's not stored in the reducer yet - we need to lift the "autofillEnabled" state into the builder state so we can change it there. EDIT: rerunautofiller and most constrained are trickier yet. We could punt on those for now
  • I am OK with these keybindings in principle but in practice I think a chorded solution like the one outlined above offers more flexibility for adding a bunch of keybindings long term. I guess the only exception to that in general is that if there is a key-binding the NYT browser app supports then we should also support it.
  • The solution outlined above might obviate the need to add modifier to the key interface, since we'd mostly be relying on sequences of unmodified keys
  • Icons have been cleaned up a bit w/ my commit above, I think.
  • Let's keep shift-arrow nav as a separate issue for now just to keep the scope of this a bit smaller. But I agree that I'd like to get that working soon.
  • Escape working on every menu is a good idea I think.

One other note is that the current implementation above currently swallows all modified browser commands, like ctrl-r to refresh or ctrl-+ to change font size. That would be fixed by the chording suggestion above but if we decide not to do that we need to limit which modified cmds get swallowed. EDIT - this happened in the last changeset, not this one

@1Computer1
Copy link
Contributor Author

1Computer1 commented Dec 14, 2021

  • We definitely should move this logic into the reducer - most of it is pretty straight forward. For example, instead of dispatching a check cell action when ! is pressed, we dispatch a regular keystroke action and add a case statement in the reducer for the ! keystroke that just does exactly what the check cell action does currently. I think the only thing that requires more changes is disabling autofill because it's not stored in the reducer yet - we need to lift the "autofillEnabled" state into the builder state so we can change it there. EDIT: rerunautofiller and most constrained are trickier yet. We could punt on those for now

As an alternative, what if the keypress handling takes place in the component, and that dispatches the correct action? I think that might be a good separation of concerns. So e.g. Puzzle sees UpArrow is pressed and dispatches a GridAction that moves up.

@mdirolf
Copy link
Member

mdirolf commented Dec 14, 2021

As an alternative, what if the keypress handling takes place in the component, and that dispatches the correct action? I think that might be a good separation of concerns. So e.g. Puzzle sees UpArrow is pressed and dispatches a GridAction that moves up.

I think this is a reasonable suggestion - as long as we can find a way to still do code sharing for all the overlap between the various grid views (puzzle, builder, etc). I'm sure we can. The only hiccup with this approach is that the keyboard handler will need some state now to track where we are in a chord, assuming we go with the chording approach (which I'm definitely leaning towards). So we'll just need to think about the right way to structure that so it's easy to share across the different components. Maybe a react hook so we can have state and it returns a callback we register as the event handler. I'm sure there's a cleaner way, that's the first thought that came to me.

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.

Keyboard shortcuts
2 participants