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

Add chord/sequence support to mapper tool #91

Merged
merged 9 commits into from
Sep 12, 2023
Merged

Add chord/sequence support to mapper tool #91

merged 9 commits into from
Sep 12, 2023

Conversation

iansan5653
Copy link
Member

@iansan5653 iansan5653 commented Sep 8, 2023

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.

@iansan5653 iansan5653 marked this pull request as ready for review September 11, 2023 13:24
@iansan5653 iansan5653 requested review from theinterned and removed request for srt32 September 11, 2023 13:24
@iansan5653 iansan5653 changed the title Add chord support to mapper tool Add chord/sequence support to mapper tool Sep 11, 2023
Copy link
Contributor

@theinterned theinterned left a 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

Comment on lines +22 to +27
- name: Copy files
run: |
mkdir _site
cp -r pages _site
cp -r dist _site

Copy link
Contributor

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.

Copy link
Member Author

@iansan5653 iansan5653 Sep 12, 2023

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[] = []
Copy link
Contributor

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.

Copy link
Member Author

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

Copy link
Contributor

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

@iansan5653 iansan5653 merged commit aa78cee into main Sep 12, 2023
2 checks passed
@iansan5653 iansan5653 deleted the chord-mapper branch September 12, 2023 21:07
theinterned added a commit that referenced this pull request Oct 10, 2023
given changes from #91 this change was no longer relevant
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