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

Command key override #35

Merged
merged 1 commit into from
Oct 9, 2018
Merged

Conversation

amychan331
Copy link
Collaborator

The command key is now override - as long as the other keys are pressed first. For example, Cmd-H would work if the H key is pressed down before Cmd is pressed. Also fixed a lot of the indentations.

@vegetabill
Copy link
Collaborator

Hi Amy, thanks for putting this together! I'm happy to see lots of different folks contributing to this helpful tool for the future students.

I appreciate you fixing the formatting. If you'd like to add an eslint config or your preferred formatting system, by all means go for it! But for this PR would you mind splitting your single commit into two so that one is for the formatting and a second is for the functionality change? This way it will be easier to review the important part of this PR as well as make it easier to run commands like git blame in the future. If you haven't done this a lot before and would like some tips, this Stack Overflow topic has some examples listed.

As for the fix itself, I had some trouble verifying it on my machine. On your PR branch, whether I pressed Cmd before or after the other key involved in the solution, the other key would become "stuck" in the pressed orange style. See this example screencast. This was on Mac OSX latest Chrome browser. Did you experience anything like this during your testing?

@amychan331 amychan331 merged commit 9a6ebde into Techtonica:master Oct 9, 2018
@amychan331
Copy link
Collaborator Author

The indentation was fixed in a separate pull request at #36. For some strange reason, this branch merged at the same time, so I will probably have to open another pull request to get this fix.
As the key getting stuck, it did indeed happened during my test - but also before I made the changes. I believe it is related to issue #25, which I am not planning to work on right now.

@vegetabill
Copy link
Collaborator

Thanks for splitting those commits. As for the "sticking", I'm not sure then. Which keys did you see sticking before your changes? I don't think I saw any issues like that on master, outside of the specific one I reported which I believe is related to the tab losing focus and getting out of sync. If I stayed on the same tab, there was no problem. Although the symptoms do sound very similar, I'm not sure it's the same root cause.

Sounds like it was unintentional, but it's a good habit not to merge your own PRs in the future. If no one is responding, feel free to ping people in #keyboard-practice-app.

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.

None yet

2 participants