-
Notifications
You must be signed in to change notification settings - Fork 97
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
Add chord/sequence support to mapper tool #91
Conversation
3280e40
to
8fc3e45
Compare
8fc3e45
to
76ae61b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There’s one nit, but besides looks great.
I actually took a bit of a crack at seeing if we could do this without the sequence tracker, and I agree it seems necessary
- name: Copy files | ||
run: | | ||
mkdir _site | ||
cp -r pages _site | ||
cp -r dist _site | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(kinda tangential but ...)
it would be nice if the action and local dev were in sync... but I guess we don't need to run locally from a _site
directory. Maybe we just need instructions on running locally.
I've been running locally via npx serve .
... not sure about you? It might be nice to add serve
as a dev dependency and add instructions in the README about running locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yeah that would be useful. I've been running npm run build
and then serving with the VSCode live preview, which is a bit cumbersome, especially since build
doesn't support a watch mode. Maybe in a future PR?
export default class SequenceTracker { | ||
static readonly CHORD_TIMEOUT = 1500 | ||
|
||
private _path: readonly string[] = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit nit, but the underscore seems superfluous and inconsistent with the other private fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I needed some way to distinguish this from the public path
getter. Would you prefer a different naming approach?
I did try using real private class fields (#path
) here but ran into some compilation issues due to using an old TS version. I think it wouldn't be hard to resolve but I didn't want to get too into the weeds in this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah didn't realize that we didn't have proper #private
fields. Carry on then! This makes sense
given changes from #91 this change was no longer relevant
Adds sequence support to the mapper utility, along with an indicator for the timer and a Reset button. The reset button is useful to be able to clear keys since otherwise it will just keep adding presses (unless the user waits 1.5 seconds).
Screen.Recording.2023-09-08.at.3.19.58.PM.mov
This is powered by a new
SequenceTracker
class which contains the sequence timer to ensure we keep things consistent across the library and the mapper.In addition, this PR sets up Actions to deploy Pages so that we don't have to import from unpkg for the pages demos. This allows us to not publish the
SequenceTracker
class publicly because we can now import directly from our own JS code.Important
The Actions workflow requires a repo settings change in https://github.com/github/hotkey/settings/pages, which I don't have access to. All we need to do is change the Source to Actions.