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 issue with incorrect x and y for rotated key #1

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

Conversation

kevin-wells
Copy link

kle-serial behaves differently to keyboard-layout-editor for rotated keys, for example the E key in the default Atreus layout.

Changed include a test to replicate the problem, and a fix which does the same thing as keyboard-layout-editor.

I also added es5 and es6 to libraries because otherwise npm run build would not work for me.

Copy link

@nooges nooges left a comment

Choose a reason for hiding this comment

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

Tested this out with my project, works for me.

@chr11031
Copy link

@kevin-wells I have a differently coded fix for this. Would your PR benefit from any of my code?
https://github.com/ijprest/kle-serial/compare/master...chr11031:chr11031_rotation_x_y_hotfix?expand=1

@kevin-wells
Copy link
Author

@chr11031 Is this fixing a bug e.g. a layout that works with keyboard-layout-editor but not kle-serial? I've been treating https://github.com/ijprest/keyboard-layout-editor/blob/master/serial.js as the specification.

@chr11031
Copy link

@kevin-wells That is correct. In rendering KLE data parsed from KLE-Serial I found these changes necessary to address the rotation issues I believe your PR/Issue is covering.

I saw that you might be working on something similar to my challenge so I'm sharing these changes with you. I shared it in the hope that maybe it would prove useful to you. If I'm misreading things though or it is not relevant to your interests, no loss in ignoring my contribution.

@haversnail
Copy link

haversnail commented Jan 18, 2023

@ijprest is this something we can get merged and published to npm? Came across this issue as well with an Alice layout.

Also tested the change and it works for my case as well, FWIW. 👍

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.

4 participants