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 pressing functional keys and add missed chars #185

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

wKich
Copy link
Member

@wKich wKich commented Feb 17, 2022

Motivation

There was problem with pressing functional keys, when instead of just sending keyDown and keyUp events the Keyboard interactor appends key code as a value to a text field

Approach

Reworked keyboard layout inspired by user-event library and added some missing chars
Added list of functional keys to filter input event

@wKich wKich requested a review from cowboyd February 17, 2022 13:57
@changeset-bot
Copy link

changeset-bot bot commented Feb 17, 2022

🦋 Changeset detected

Latest commit: d93827d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 5 packages
Name Type
@interactors/globals Patch
@interactors/keyboard Patch
@interactors/core Patch
@interactors/html Patch
@interactors/material-ui Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@netlify
Copy link

netlify bot commented Feb 17, 2022

✔️ Deploy Preview for interactors canceled.

🔨 Explore the source changes: d93827d

🔍 Inspect the deploy log: https://app.netlify.com/sites/interactors/deploys/622f67868414a900080fc433

| "Backslack"
| "Backslash"
Copy link
Member

Choose a reason for hiding this comment

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

🤣

| "BracketRight"
| "Quote";

export const FunctionalKeys: KeyCode[] = [
Copy link
Member

Choose a reason for hiding this comment

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

Let's just go ahead and import all of the different key types from MDN https://developer.mozilla.org/en-US/docs/Web/API/KeyboardEvent/key/Key_Values and name them after what is found there.

This way, we will be sure and have an exhaustive list, and the category of key will be aligned with what is found there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to make each key type in its own array so that we can filter accordingly? I'm not sure what "FunctionalKeys" means or if maybe it should be a combination of smaller categories of key code

Copy link
Member Author

Choose a reason for hiding this comment

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

There are two different properties of a KeyboardEvent: key and code. Here for FunctionalKeys and KeyCode we described codes and real keys defined in layouts (like for example in defaultKeyMap for US layout). But for functional keys key and code almost the same and I need them to filter as non-printable characters.

In reality it's more complicated, like backspace removes the last character, but we don't have such functionality.

So what about KeyValues they are described key property, but I don't know what code they should have. And I think I got your point. We don't filter all these key values as non-printable characters.

@wKich wKich requested a review from cowboyd March 10, 2022 12:17
@cowboyd
Copy link
Member

cowboyd commented Mar 14, 2022

I just noticed that the keys were going into the globals package. Why is that required? The reason I ask is that I believe we want to keep the globals as slim as possible.

@wKich
Copy link
Member Author

wKich commented Mar 14, 2022

Yeah, we can move non-printable keys set out to the keyboard package

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