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 numbers to keyFromKeyCode #391

Merged
merged 2 commits into from
Jul 9, 2018
Merged

Conversation

localpcguy
Copy link
Contributor

Ran into an issue where I needed the numbers to in the keycode list as well (height/weight form).

Fixes #390

@ro0gr
Copy link
Contributor

ro0gr commented Jun 29, 2018

Adding a number key test-case seems good

@rwjblue rwjblue requested review from cibernox and Turbo87 June 29, 2018 21:54
@rwjblue
Copy link
Member

rwjblue commented Jun 29, 2018

Ya, definitely needs a test.

Also, would like @cibernox and @Turbo87 to double check.

@cibernox
Copy link
Contributor

Looks good to me, it just needs tests.
Those keyCodes are for the number above the keyboard, do you know if the keycodes of the numpad keys are different? Just curiosity.

@localpcguy
Copy link
Contributor Author

The keyCodes for number pad are the same. I'll get on the test(s)

@localpcguy
Copy link
Contributor Author

localpcguy commented Jun 30, 2018

Added tests for numbers. Let me know if there are any more concerns.

@localpcguy
Copy link
Contributor Author

localpcguy commented Jul 2, 2018

@cibernox I wanted to clarify re:keycodes between the top row and number pad:

The ASCII code (event.which) is the same between both, and that is what this codes uses as the keyCode. The event.code value IS different (at least on MacOS), showing Numpad1 vs. Digit1 for the 1 key for example. That won't affect this code as we are using the event.key value when this code references keyCode, not the event.code value. See http://keycode.info/ from @wesbos for a simple way to see key information.

@rwjblue rwjblue merged commit a4253d1 into emberjs:master Jul 9, 2018
@rwjblue
Copy link
Member

rwjblue commented Jul 9, 2018

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants